Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strip ./ from discover_lintables results #1837

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

sebix
Copy link
Contributor

@sebix sebix commented Jan 28, 2022

calling WcMatch('.', ...) causes the resulting file names to be prefixed
with ./.
If the tests are run in a git-repository, this is not an issue, as git
is preferred of WcMatch in the code.

Therefore remove the ./ prefix from the WcMatch return value to get
the identical expected output.

fixes #1836

@sebix sebix requested a review from a team as a code owner January 28, 2022 18:28
@sebix sebix requested review from relrod, cidrblock and priyamsahoo and removed request for a team January 28, 2022 18:28
@github-actions github-actions bot added the bug label Jan 28, 2022
@sebix sebix force-pushed the fix_test_discover_lintables_umlaut branch 2 times, most recently from 94ec64a to fadda4a Compare January 28, 2022 18:37
@sebix
Copy link
Contributor Author

sebix commented Jan 28, 2022

src/ansiblelint/file_utils.py:267: error: Returning Any from function declared to return "str"

I'm not sure what's the best way to make mypy happy. out will still always contain only strings, not anything else. Applying a function (with string -> string) does not change the type.

@ssbarnea ssbarnea changed the title fix discover_lintables: strip ./ from WcMatch return value Ensure that discover_lintables does not return results starting with ./ Feb 1, 2022
@ssbarnea ssbarnea changed the title Ensure that discover_lintables does not return results starting with ./ Strip ./ from discover_lintables results Feb 1, 2022
@ssbarnea ssbarnea enabled auto-merge (squash) February 1, 2022 15:12
src/ansiblelint/file_utils.py Outdated Show resolved Hide resolved
src/ansiblelint/file_utils.py Outdated Show resolved Hide resolved
auto-merge was automatically disabled February 1, 2022 18:52

Head branch was pushed to by a user without write access

@sebix sebix force-pushed the fix_test_discover_lintables_umlaut branch from d299190 to 2a900ef Compare February 1, 2022 18:52
@sebix
Copy link
Contributor Author

sebix commented Feb 1, 2022

Thanks for the feedback. I refactored the code as suggested with a separate function and a set comprehension. That also obsoleted the import of typing.Callable which I reverted therefore.

Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in src/ansiblelint/file_utils.py look good to me.

I'm not sure what the other changes are about though. They seem to be unrelated to this PR.

@sebix sebix force-pushed the fix_test_discover_lintables_umlaut branch from 5155d8e to 116015f Compare February 1, 2022 19:05
@sebix
Copy link
Contributor Author

sebix commented Feb 1, 2022

I'm not sure what the other changes are about though. They seem to be unrelated to this PR.

Now rebase on updated main. Looks better :)

calling WcMatch('.', ...) causes the resulting file names to be prefixed
with `./`.
If the tests are run in a git-repository, this is not an issue, as git
is preferred of WcMatch in the code.

Therefore remove the `./` prefix from the WcMatch return value to get
the identical expected output.

fixes ansible#1836
@sebix sebix force-pushed the fix_test_discover_lintables_umlaut branch from 116015f to f881ebf Compare February 1, 2022 19:37
@sebix
Copy link
Contributor Author

sebix commented Feb 1, 2022

... and now also fixed this check fail:

src/ansiblelint/file_utils.py:276:1: D400 First line should end with a period

@ssbarnea ssbarnea merged commit 79a91c6 into ansible:main Feb 2, 2022
@sebix sebix deleted the fix_test_discover_lintables_umlaut branch February 2, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests: Filenames returned by wcmatch prefixed with ./ cause fails
3 participants