Fix pep8 compatibility code (fixes #501) #507
Conversation
* When using the `replaced_by_pep8` decorator with a different method signature for simplicity, as the real signature is generated by the decorator, some static checkers might find false positive errors when calling pyparsing code (see issue pyparsing#501). * Refactor the calls for the compatibility code to not use the `replaced_by_pep8` decorator but calling directly the `_make_synonym_function` function to generate the synonym methods/functions. * This way the signature of the method/function is automatically copied and the static checkers will not report false positives.
|
Thanks - I'm reluctant to drop the Just wanted to make you aware of some of the other issues around this topic. |
As far as I can tell the deprecation warning calls are already inside the $ python -Walways
Python 3.10.11 (main, Apr 7 2023, 07:33:46) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import pyparsing as pp
./pyparsing/common.py:8: DeprecationWarning: Deprecated - use set_name
class pyparsing_common:
>>> greet = pp.Word(pp.alphas) + "," + pp.Word(pp.alphas) + "!"
>>> greet.parseString("Hello, World!")
sys:1: DeprecationWarning: Deprecated - use parse_string
ParseResults(['Hello', ',', 'World', '!'], {})
>>> greet.parse_string("Hello, World!")
ParseResults(['Hello', ',', 'World', '!'], {})
>>> greet.parseString("Hello, World!")
sys:1: DeprecationWarning: Deprecated - use parse_string
ParseResults(['Hello', ',', 'World', '!'], {})
>>>
The code taking care of that is also part of >>> greet.parseString.__doc__
'Deprecated - use :class:`parse_string`'The vs If it's just a problem of naming of course we can rename |
|
@ptmcg when possible let me know what are your thoughts on this |
|
Thanks for verifying that the doc gen behavior is preserved. I'm not sure why I wrapped This is a lot simpler of a change if we just rename Lastly, now that the wrapper is gone, the DeprecationWarnings (which are still comments, but hopefully won't be for much longer) currently use stacklevel=3 assuming this would be called in the wrapper function - now that that is being removed, please change to stacklevel=2, so that I don't forget to when I eventually enable these. |
| @@ -275,10 +275,3 @@ def _inner(*args, **kwargs): | |||
| _inner.__kwdefaults__ = None | |||
| _inner.__qualname__ = fn.__qualname__ | |||
| return cast(C, _inner) | |||
|
|
|||
|
|
|||
| def replaced_by_pep8(fn: C) -> Callable[[Callable], C]: | |||
There was a problem hiding this comment.
Instead of deleting this and changing all the references to directly reference _make_synonym_function, please just rename _make_synonym_function to replaced_by_pep8
There was a problem hiding this comment.
Also, please change the commented line 263 to "stacklevel=2", so that I don't forget later when I uncomment this code.
There was a problem hiding this comment.
Thanks for the review, I should have addressed your comments in the last commit.
* Rename the `_make_synonym_function` function to `replaced_by_pep8` for clarity. * Reduce the stacklevel of the commented out deprecation warnings accordingly.
|
@ptmcg Is there anything else missing that I should add/change in this pull request? |
|
I think this all looks good. Thanks for your patience and diligence. |
replaced_by_pep8decorator with a different method signature for simplicity, as the real signature is generated by the decorator, some static checkers might find false positive errors when calling pyparsing code (see issue Pylint errors on Pyparsing v3.1.0 with old-style method/argument names #501).replaced_by_pep8decorator but calling directly the_make_synonym_functionfunction to generate the synonym methods/functions.