Support user-provided fixers (fixes #166) #167
Conversation
There was a problem hiding this comment.
Thanks, I think this makes sense overall. I've made a few comments inline, mostly about the documentation.
The fixer-selection code is getting complex enough that I think it would be good to have at least a couple of tests for that. That needn't be your responsibility, but you're here at the moment and looking keen, so... 🐕
libmodernize/main.py
Outdated
| parser.add_option("-d", "--doctests_only", action="store_true", | ||
| help="Fix up doctests only.") | ||
| parser.add_option("-f", "--fix", action="append", default=[], | ||
| help="Each FIX specifies a transformation; '-f default' includes default fixers.") | ||
| parser.add_option("-H", "--fixers-here", action="store_true", |
There was a problem hiding this comment.
This is totally a matter of taste, but I lean towards having only the long option for this, so people have to spell it out. It's much clearer that way. I quite like the name --fixers-here
docs/fixers.rst
Outdated
| @@ -21,6 +21,12 @@ then use the ``--no-six`` flag. | |||
| Fixers use the API defined by 2to3. For details of how this works, and how to | |||
| implement your own fixers, see `Extending 2to3 with your own fixers, at | |||
| python3porting.com <http://python3porting.com/fixers.html>`_. | |||
| ``python-modernize`` will try to load fixers whose full dotted-path is specified | |||
| as a ``-f`` argument, but will fail if they are not found. Note that, by | |||
There was a problem hiding this comment.
Something I picked up from time spent editing Wikipedia: the words "Note that" can almost always be deleted with no change to the meaning. And indeed that works here!
docs/fixers.rst
Outdated
| default, the current directory is not on the Python-Path when you run the | ||
| command; the ``-H``/``--fixers-here`` option is provided in order to add it, | ||
| as a shorthand for explicit manipulation of the ``PYTHONPATH`` environment | ||
| variable. |
There was a problem hiding this comment.
I'd cut it off before "as a shorthand". If people know about PYTHONPATH, they don't need telling, and if they don't it's a distraction from what we're trying to explain.
docs/fixers.rst
Outdated
| @@ -21,6 +21,12 @@ then use the ``--no-six`` flag. | |||
| Fixers use the API defined by 2to3. For details of how this works, and how to | |||
| implement your own fixers, see `Extending 2to3 with your own fixers, at | |||
| python3porting.com <http://python3porting.com/fixers.html>`_. | |||
| ``python-modernize`` will try to load fixers whose full dotted-path is specified | |||
| as a ``-f`` argument, but will fail if they are not found. Note that, by | |||
| default, the current directory is not on the Python-Path when you run the | |||
There was a problem hiding this comment.
Rather than mentionining the Python path, I'd say something like "...fixers will not be found in the current directory..."
There was a problem hiding this comment.
I think it would be incomplete not to say anything about the Python path here, but I see your point.
How about something like,
By default, fixers will not be found in the current directory;
use ``--fixers-here`` to make ``python-modernize`` look for them there,
or see (link to relevant docs) for more info on how Python finds modules.There was a problem hiding this comment.
That sounds good, though I don't know of any good explanatory docs for how imports are found. This page is much too detailed.
There was a problem hiding this comment.
|
(The 🐕 is meant to be looking imploringly at you about tests. That's probably not obvious outside my head. Sleep beckons.) |
|
The 🐕 is fine (frankly, it's small and yellow and if I had seen the emoji without seeing the text form in the mail first, I wouldn't even be sure what animal it was, but since I did see the text form, it made perfect sense), and tests are important to me personally, but I don't think I can do them on Matific's dime, so it would take more time for me to do them. If you don't consider them critical, i'd rather open a separate issue for them, and avoid blocking this PR for them. |
|
Fair enough, thanks @shaib :-) (If they're willing to pay you for time improving open source software - which is definitely a good policy - I think they should see writing tests as an important part of that. But I appreciate that you might not be in a position to make that argument) |
Avoid erroring out when a non-standard fixer is specified, and instead hand it over to the refactoring tool so it can try to load it. While at it, add a command-line option to add the current directory to sys.path, to make it easier for users to let their fixers be found.
|
This should address the review comments |
|
Thanks! |
|
Opened #168 for a test for this logic. |
This adds support for user-provided fixers in the
python-modernizecommand -- or, rather, mostly, removes the blocking of their use.I found that, when the command-line tool is invoked, the current working directory is not on the Python-path -- which means user-provided fixers are likely not to be found. I added a command-line flag to help remedy this, thinking that by default, we don't want random modules to mix into the library.
I made what I think are the relevant documentation changes, criticism very welcome there. English is not my native language.
I looked for tests, and found essentially none for similar functionality of the main module, so I skipped those; if you think it necessary, I'll add them.
I also included a separate commit which prevents a single
-foption from referring to more than one fixer; this possibility was introduced with the support for short names. It is not a condition for user-provided fixers, each can be accepted or rejected separately.This work was sponsored by Matific at https://matific.com