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

Use cross-platform regexes for some State.looks_like* functions #286

Conversation

CLWilliam
Copy link
Contributor

Fixes #283

  • looks_like_migrations_file and looks_like_command_file are converted to regex based filename checks.
  • Tests for looks_like_migrations_file added
  • git example added for powershell to apply django-upgrade on multiple files

@CLWilliam CLWilliam changed the title Issue 283 handling different path separators Change looks_like_migrations_file & looks_like_command_file to regex based checks Nov 3, 2022
Copy link
Owner

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

Thanks this is looking good.

Please add a changelog note in HISTORY.rst following the existing style (whenever you make an open source contribution.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Comment on lines 35 to 36
migrations_re = re.compile(r"(^|[\\/])migrations([\\/])")
commands_re = re.compile(r"(^|[\\/])management[\\/]commands[\\/]")
Copy link
Owner

Choose a reason for hiding this comment

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

please sort the regexes alphabetically, the same order that the methods are in

The commands method did deliberately check the current file lives within commands, which the regex currently doesn’t. I think we could modify it like so to capture this:

commands_re = re.compile(r"(^|[\\/])management[\\/]commands[\\/][^\\/]+$")

Since you’re adjusting commands_re, please ensure it is tested with both \ and /

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one fails on myapp/subapp/management/commands/test/subcommand.py as it has a subfolder and thus an additional path separator. expanding it to

(^|[\\/])management[\\/]commands[\\/][^\\/]+[\\/]*[^\\/]*$

captures one subfolder. Pushing it further to 0 or more subfolders brings:

(^|[\\/])(management[\\/]commands[\\/])([^\\/]+[\\/]+)*([^\\/]+)$

--> regexr.com/71kmn

Commands tests are also updated for windows paths.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that was a deliberate suggestion - subdirectories won't be picked up as management commands by Django, so we can ignore certain fixers there.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah actually my bad, I didn't realize subcommand was in the existing tests. We can keep support for subcommand. Reverting to your original easier regex since the 0+ folders search is verbose and doesn’t help much.

@adamchainz adamchainz changed the title Change looks_like_migrations_file & looks_like_command_file to regex based checks Use cross-platform regexes for some State.looks_like* functions Nov 3, 2022
CLWilliam added a commit to CLWilliam/django-upgrade that referenced this pull request Nov 4, 2022
@adamchainz adamchainz force-pushed the Issue_283-Handling_different_path_separators branch from f90753c to 140e35c Compare November 9, 2022 12:00
@adamchainz adamchainz merged commit c9ac95c into adamchainz:main Nov 9, 2022
@adamchainz
Copy link
Owner

Thank you!

@CLWilliam
Copy link
Contributor Author

Thanks for the guidance! It was a nice way to see the full issue to merge cycle!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NullBooleanField upgraded to BooleanField in migrations file
2 participants