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

Replace optparse by argparse #2952

Merged
merged 6 commits into from May 20, 2019

Conversation

realead
Copy link
Contributor

@realead realead commented May 15, 2019

Right now there are at least three different option/arguments parser:

1. parser for cython.py (uses no external libraries)
2. parser for cythonize.py (uses optparse)
3. parser for IPython-magic (based on argparse)

For adding/changing new options one has to understand all 3 way, there are also some discrepancies - for example -3str option is missed in IPython-magic's command line.

Since Cython no longer supports Python2.6 it is possible to use argparse and drop deprecated optparse.

The only behavior chance of this PR is a slightly different help message.

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing this, this is much nicer and cleaner than before – and even tested :).
Thanks also for keeping such a nice diff for the options.

Cython/Build/Tests/TestCythonizeArgsParser.py Outdated Show resolved Hide resolved
self.assertFalse(args)
self.assertTrue(self.are_default(options, ['options']))
self.assertEqual(options.options['docstrings'], True)
self.assertEqual(options.options['buffer_max_dims'], True) # really?
Copy link
Contributor

Choose a reason for hiding this comment

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

really?

Hmm, no, certainly not, totally worth fixing. Thanks for testing it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, here I got confused. Because --options are not for Compiler-options (which buffer_max_dims is, see https://cython.readthedocs.io/en/latest/src/userguide/source_files_and_compilation.html#compiler-options), but then it is not really documented (or I have missed this part) what those options are. They seems to accept any keyword (fe7effd#diff-04f8641896f9a54d3de3abf279f486c4R208) but it can be only true or false.

from Cython.TestUtils import CythonTest


class TestCythonizeArgsParser(CythonTest):
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of error tests for the value validation would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understood what you mean, but I have added some more tests testing the behavior of parse_directives and parse_options.

setattr(namespace, self.dest, directives)

class ParseOptionsAction(Action):
def __call__(self, parser, namespace, values, option_string=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make use of the option type here? That could solve the buffer_max_ndims problem.

Copy link
Contributor Author

@realead realead May 19, 2019

Choose a reason for hiding this comment

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

I would like to keep this PR without behavioral changes (also because I think it could become a bigger thing as one might expect, see also #2952 (comment)). So not sure about this.

'auto_pickle': 'NONONO', # for bool type
'c_string_type': 'bites',
#'c_string_encoding' : 'a',
#'language_level' : 4,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would expect the parser to raise an error for -X language_level=4 but it doesn't currently.

@scoder scoder merged commit 2a1ec5e into cython:master May 20, 2019
@scoder scoder added this to the 3.0 milestone May 20, 2019
@scoder
Copy link
Contributor

scoder commented May 20, 2019

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants