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

Re-harmonize argument parsing between distutils and optparse CLIs #389

Merged
merged 5 commits into from Apr 19, 2016

Conversation

@akx
Copy link
Member

@akx akx commented Apr 14, 2016

I so hope this will be the last we see of this series of bugs...

Fixes #388
Fixes #390
Refs #384
Refs #311

@akx akx added the bug label Apr 14, 2016
@codecov-io
Copy link

@codecov-io codecov-io commented Apr 14, 2016

Current coverage is 90.10%

Merging #389 into master will increase coverage by +0.02% as of aecf236

@@            master   #389   diff @@
=====================================
  Files           24     24       
  Stmts         3935   3950    +15
  Branches         0      0       
  Methods          0      0       
=====================================
+ Hit           3545   3559    +14
  Partial          0      0       
- Missed         390    391     +1

Review entire Coverage Diff as of aecf236

Powered by Codecov. Updated on successful CI builds.

@akx
Copy link
Member Author

@akx akx commented Apr 14, 2016

Assigning @jtwang just to have some eyes on this.

@ENuge
Copy link
Contributor

@ENuge ENuge commented Apr 14, 2016

(Pretend I'm jtwang), shipit! I'm not super familiar with the different ways users can do option parsing, but the logic looks fine and you've got it tested. Hopefully this doesn't introduce any more regressions. :)

@akx
Copy link
Member Author

@akx akx commented Apr 15, 2016

Well, now that #390 appeared, I'm going to add a take a super duper close look at every single parameter that may have been mutated in #311. So re-review after that's done . . .

@akx akx unassigned jtwang Apr 15, 2016
@akx akx changed the title Harmonize extraction keyword parsing between distutils and standalone CLI Re-harmonize argument parsing between distutils and optparse CLIs Apr 17, 2016
@akx akx force-pushed the akx:fix-388 branch from f070e9f to 97c1d18 Apr 17, 2016
@akx
Copy link
Member Author

@akx akx commented Apr 17, 2016

Ping @ENuge / @jtwang again.

@@ -39,6 +39,48 @@
from configparser import RawConfigParser


def listify_value(arg, split=None):

This comment has been minimized.

@jtwang

jtwang Apr 17, 2016
Contributor

Will a single arg ever contain quotes/whitespace? Eg "foo "i am a.zip" bar"

This comment has been minimized.

@akx

akx Apr 17, 2016
Author Member

That sort of usage will never have been (how's that for a temporal form?) supported by Babel... At least not the distutils frontend. (Sneaky edit: distutils, not optparse)

Double edit: Keyword strings should never contain significant whitespace, so that shouldn't matter, actually. Directory/path args are separated by other characters.

@akx
Copy link
Member Author

@akx akx commented Apr 19, 2016

@ENuge: Mind taking one more look?

@@ -825,6 +882,7 @@ def _configure_command(self, cmdname, argv):
strs = ["--%s" % name]
if short:
strs.append("-%s" % short)
strs.extend(cmdclass.option_aliases.get(name, ()))

This comment has been minimized.

@ENuge

ENuge Apr 19, 2016
Contributor

Checking if this, combined with strs = ["--%s" % name], doesn't cause any problems? If name is "output" you'll end up with strs being ['--output', --output']. I don't have a super solid understanding of all the different options uh, options, so for all I know this intentional. If not an issue, shipit! Everything else looks good to me.

This comment has been minimized.

@akx

akx Apr 19, 2016
Author Member

Oop, good catch.

The alias mapping should be 'output-file': ('--output',)...

This comment has been minimized.

@akx

akx Apr 19, 2016
Author Member

(and as of ee8abd6, it is.)

This comment has been minimized.

@ENuge

ENuge Apr 19, 2016
Contributor

Err, oh. I don't know why I was looking at the old version. Shipit!

akx added 5 commits Apr 14, 2016
Now that arguments are more likely to be unicode than bytestrings, %r is not a good idea between Python versions.
@akx akx force-pushed the akx:fix-388 branch from 97c1d18 to ee8abd6 Apr 19, 2016
@akx
Copy link
Member Author

@akx akx commented Apr 19, 2016

So yeah, with that last comment fixed, merging. Will roll another bugfix release tomorrow.

@akx akx merged commit 0e71d20 into python-babel:master Apr 19, 2016
6 of 7 checks passed
6 of 7 checks passed
review/gitmate/manual This commit needs review.
Details
codecov/patch 96.00% of diff hit (target 80.00%)
Details
codecov/project 90.10% (+0.02%) compared to 95c86d5 at 90.08%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
review/gitmate/commit No issues with this one - go ahead! :)
Details
review/gitmate/pr All is well! :) (0 problems solved)
Details
@akx akx deleted the akx:fix-388 branch Apr 19, 2016
@akx akx mentioned this pull request Apr 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.