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

Dry run mode #72

Merged
merged 9 commits into from Aug 9, 2020
Merged

Dry run mode #72

merged 9 commits into from Aug 9, 2020

Conversation

cphyc
Copy link
Contributor

@cphyc cphyc commented Aug 7, 2020

This adds a dry-run mode and fixes #48.

README.md Outdated
@@ -42,6 +42,8 @@ optional arguments:
--no-multiline convert only single line expressions
-ll LINE_LENGTH, --line-length LINE_LENGTH
for expressions spanning multiple lines, convert only if the resulting single line will fit into the line length limit. Default value is 88 characters.
-n, --dry-run Do not change the file in-place. Useful when running
Copy link
Owner

Choose a reason for hiding this comment

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

why -n? I am using the first letter as a short-hand for each flag; -d would be more consistent (and intuitive imo)

src/flynt/cli.py Outdated
"--dry-run",
action="store_true",
default=False,
help="Do not change the file in-place. Useful when running as a linting process."
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps it's useful to elaborate that it MUST be used with --fail-on-change, otherwise flynt just does nothing and returns nothing or potentially fails.

@ikamensh
Copy link
Owner

ikamensh commented Aug 7, 2020

Thanks for your contribution!
few remarks:

  1. Issue feature request: dryrun mode? #48 was proposing to e.g. print the diff to the stdout, which this version of the feature is not doing yet.
    would you like to expand this contribution to print the diff?

  2. let's print a message at the beginning stating the changed behavior. Something like "Flynt is running in dry mode. Files will not be changed." Reasoning: prevent any confusion.

@cphyc
Copy link
Contributor Author

cphyc commented Aug 8, 2020

Thanks for the feedback! Does this looks reasonable to you?

  • I added a small message when running in dry-run mode to inform no files will be touched
  • a diff-style message is printed instead of writing to files
  • the user is advised to use --fail-on-change when using --dry-run for linting purposes.

For example on the CI of https://github.com/yt-project/yt, this produces

$ flynt --fail-on-change --dry-run yt/
Running flynt v.0.49
Running flynt in dry-run mode. No files will be changed.
--- /home/travis/build/yt-project/yt/yt/exthook.py
+++ 
@@ -86,7 +86,7 @@
             if "." not in modname:
                 setattr(sys.modules[self.wrapper_module], modname, module)
             return module
-        raise ImportError("No module named %s" % fullname)
+        raise ImportError(f"No module named {fullname}")
 
     def is_important_traceback(self, important_module, tb):
         """Walks a traceback's frames and checks if any of the frames
[...]
flynt run has finished. Stats:
Execution time:                            11.892s
Files modified:                            205
Character count reduction:                 4773 (0.08%)
Per expression type:
Old style (`%`) expressions attempted:     1038/1298 (80.0%)
`.format(...)` calls attempted:            125/126 (99.2%)
No concatenations attempted.
F-string expressions created:              1081
Out of all attempted transforms, 248 resulted in errors.
To find out specific error messages, use --verbose flag.
_-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_.
Please run your tests before committing. Did flynt get a perfect conversion? give it a star at: 
~ https://github.com/ikamensh/flynt ~
Thank you for using flynt. Upgrade more projects and recommend it to your colleagues!
The command "flynt --fail-on-change --dry-run yt/" exited with 205.

Copy link
Owner

@ikamensh ikamensh left a comment

Choose a reason for hiding this comment

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

Thumbs up on picking up the diff part as well. I didn't know there was a stdlib module for handling this!

I have tried this and it works nicely. Ready to merge it, with the small comment about readme - now that we print diff the --dry-run became useful on it's own.

src/flynt/cli.py Outdated
"--dry-run",
action="store_true",
default=False,
help="Do not change the file in-place. Useful when running as a linting process."
help="Do not change the file in-place. Note that this must be used in "
Copy link
Owner

Choose a reason for hiding this comment

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

When suggesting this change, I didn't consider we will end up implementing printing diff. Now it's more relevant to state that proposed diff will be printed to stdout, and --dry-run because useful on it's own without --fail-on-change.

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 should be fixed now.

@ikamensh ikamensh merged commit d3e65f7 into ikamensh:master Aug 9, 2020
@ikamensh
Copy link
Owner

ikamensh commented Aug 9, 2020

Thanks! merged.

@cphyc cphyc deleted the dry-run-mode branch August 9, 2020 09:00
@cphyc
Copy link
Contributor Author

cphyc commented Aug 9, 2020

Do you think it would be possible to issue a release (v.50.0)?

@ikamensh
Copy link
Owner

ikamensh commented Aug 9, 2020

done: https://pypi.org/project/flynt/0.50/

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.

feature request: dryrun mode?
2 participants