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

Fix pep8 compatibility code (fixes #501) #507

Merged
merged 2 commits into from Oct 2, 2023
Merged

Conversation

volans-
Copy link
Contributor

@volans- volans- commented Aug 4, 2023

  • 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 Pylint errors on Pyparsing v3.1.0 with old-style method/argument names #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.

* 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.
@ptmcg
Copy link
Member

ptmcg commented Aug 7, 2023

Thanks - I'm reluctant to drop the replaced_by_pep8 function, since that will be the place that I start to throw DeprecationWarnings in the near future. Also, some of these changes were specifically to address a problem that we had with duplicating the complete docstring in parse_string and parseString, set_name and setName, etc. So whatever we do, we don't want to restore that duplication of docstrings (which really messed up the docs). But I'll see if there is a way we can accommodate all of these points.

Just wanted to make you aware of some of the other issues around this topic.

@volans-
Copy link
Contributor Author

volans- commented Aug 7, 2023

Thanks - I'm reluctant to drop the replaced_by_pep8 function, since that will be the place that I start to throw DeprecationWarnings in the near future.

As far as I can tell the deprecation warning calls are already inside the _make_synonym_function function. If I uncomment those in my patch they seems to work just fine:

$ 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', '!'], {})
>>>

Also, some of these changes were specifically to address a problem that we had with duplicating the complete docstring in parse_string and parseString, set_name and setName, etc. So whatever we do, we don't want to restore that duplication of docstrings (which really messed up the docs).

The code taking care of that is also part of _make_synonym_function and works fine:

>>> greet.parseString.__doc__
'Deprecated - use :class:`parse_string`'

The tox.ini doesn't provide an environment to test building the documentation, but a quick run with sphinx_build did generate it as expected:
Screenshot 2023-08-07 at 07 42 37

vs
Screenshot 2023-08-07 at 07 42 51

If it's just a problem of naming of course we can rename _make_synonym_function to replaced_by_pep8or any other more suitable name.

@volans-
Copy link
Contributor Author

volans- commented Aug 29, 2023

@ptmcg when possible let me know what are your thoughts on this

@ptmcg
Copy link
Member

ptmcg commented Sep 1, 2023

Thanks for verifying that the doc gen behavior is preserved. I'm not sure why I wrapped _make_synonym_function inside the other.

This is a lot simpler of a change if we just rename _make_synonym_function to replaced_by_pep8 - won't have to modify all the places where it is used, and the name is more descriptive.

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]:
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Also, please change the commented line 263 to "stacklevel=2", so that I don't forget later when I uncomment this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, I should have addressed your comments in the last commit.

@ptmcg ptmcg self-assigned this Sep 2, 2023
* Rename the `_make_synonym_function` function to `replaced_by_pep8` for
  clarity.
* Reduce the stacklevel of the commented out deprecation warnings
  accordingly.
@volans-
Copy link
Contributor Author

volans- commented Oct 1, 2023

@ptmcg Is there anything else missing that I should add/change in this pull request?

@ptmcg
Copy link
Member

ptmcg commented Oct 2, 2023

I think this all looks good. Thanks for your patience and diligence.

@ptmcg ptmcg merged commit 4a3a62a into pyparsing:master Oct 2, 2023
0 of 18 checks passed
@volans- volans- deleted the pylint branch October 2, 2023 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants