Skip to content

gh-130425: Add "Did you mean" suggestion for del obj.attr #130427

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

Closed
wants to merge 2 commits into from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Feb 21, 2025

I am not quite sure about the proper way to test this:

  • There are no tests for delattr and setattr suggestions, it feels like this should be a separate issue to add them. I can do it in the next PR, there would be lots of corner cases
  • Local testing works:
» ./python.exe     
Python 3.14.0a5+ (heads/main-dirty:38642bff139, Feb 22 2025, 01:18:34) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class A:
...     pass
... 
... a = A()
... a.abcde = 1
... del a.abcdf # typo
... 
Traceback (most recent call last):
  File "<python-input-0>", line 6, in <module>
    del a.abcdf # typo
        ^^^^^^^
AttributeError: 'A' object has no attribute 'abcdf'. Did you mean: 'abcde'?
>>> 

@StanFromIreland
Copy link
Member

I agree the tests should be added, a separate issue would be good :-)

Copy link
Member

@StanFromIreland StanFromIreland left a comment

Choose a reason for hiding this comment

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

Looks good now!

Pranjal095 added a commit to Pranjal095/cpython that referenced this pull request Mar 3, 2025
…gestions-combined

This merges the delattr suggestion feature implementation by @sobolevn from PR python#130427 with added tests from PR python#130455.
Pranjal095 added a commit to Pranjal095/cpython that referenced this pull request Mar 3, 2025
…to delattr-suggestions-combined

Adds test cases for the delattr suggestion feature implemented in PR python#130427 by @sobolevn.
@picnixz
Copy link
Member

picnixz commented Mar 3, 2025

I would suggest either using this one and backport the tests from #130455 or use #130799 (which backports this PR).

@sobolevn
Copy link
Member Author

sobolevn commented Mar 4, 2025

I think that the PR is quite obvious and combining it with tests is not really worth it :(

@picnixz
Copy link
Member

picnixz commented Mar 4, 2025

Well @encukou actually asked for a merged PR. I would also prefer having both tests and implementation in the same commit so to avoid double commit reversion in case of an issue we didn't forsee.

@cfbolz
Copy link
Contributor

cfbolz commented Mar 4, 2025

Please add tests. I agree that the implementation is really simple, but eg PyPy relies on the standard library tests a lot to find out what details in behaviour we are missing (and we already implemented delattr suggestions, so even more important in this specific case).

@sobolevn
Copy link
Member Author

sobolevn commented Mar 4, 2025

Ok, then - let's wait for #130799 to get finished

@sobolevn
Copy link
Member Author

sobolevn commented May 2, 2025

This was a really simple PR that collapsed under the complexity of the review process. Closing. There are now 2 alternative PRs.

@sobolevn sobolevn closed this May 2, 2025
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.

4 participants