Conversation
scoder
left a comment
There was a problem hiding this comment.
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.
| 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? |
There was a problem hiding this comment.
really?
Hmm, no, certainly not, totally worth fixing. Thanks for testing it. :)
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
A couple of error tests for the value validation would be nice.
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
Maybe make use of the option type here? That could solve the buffer_max_ndims problem.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
I would expect the parser to raise an error for -X language_level=4 but it doesn't currently.
|
Thanks! |
Right now there are at least three different option/arguments parser:
For adding/changing new options one has to understand all 3 way, there are also some discrepancies - for example
-3stroption is missed in IPython-magic's command line.Since Cython no longer supports Python2.6 it is possible to use
argparseand drop deprecatedoptparse.The only behavior chance of this PR is a slightly different help message.