Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-130425: Add "Did you mean [...]" suggestions fordel obj.attr#130799
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
…gestions-combinedThis merges the delattr suggestion feature implementation by@sobolevn from PRpython#130427 with added tests from PRpython#130455.
…to delattr-suggestions-combinedAdds test cases for the delattr suggestion feature implemented in PRpython#130427 by@sobolevn.
del obj.attrThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I also don't think that merging a fix with tests into one PR is a good idea. When we have a fix and new tests - we usually merge them as different PRs, that's the first time I see that we combine PRs :)
Lib/test/test_traceback.py Outdated
| self.assertIn("'bluch'", self.get_suggestion(A(), '_luch')) | ||
| self.assertIn("'bluch'", self.get_suggestion(A(), '_bluch')) | ||
| obj = A() | ||
| if operation == "getattr": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, I don't quite like the current design :(
It would be even more complex, when we addsetattr tests (btw, I think that they should also be added in this PR).
Can we instead create a base class out of existing testcase? And then create three subclasses which would look like so:
classGetattrSuggestionsTests(BaseSuggestionsTests,unittest.TestCase):defget_suggestion(self,obj,name):# get suggestions using `getattr`classSetattrSuggestionsTests(BaseSuggestionsTests,unittest.TestCase):defget_suggestion(self,obj,name):# get suggestions using `setattr`# the same for `delattr`
Pranjal095Mar 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll first refactor the tests as per your suggestion, and keep that as a separate commit.
I was unsure of adding setattr suggestion tests to this PR because:
- The concerning issue was raised for adding delattr suggestions alone.
- There are multiple ways to simulate an AttributeError for setattr (custom
__slots__,@propertyetc.), and I felt it would be better to discuss the appropriate testing mechanism in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have made the requested changes as per the refactor request. Kindly review the PR again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
picnixz commentedMar 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I would have actually expected that to be a feature and not a fix actually!
I actually always combined my fixes with my tests to check indeed that my fix is correct. Same for features. |
python-cla-botbot commentedApr 18, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
All commit authors signed the Contributor License Agreement. |
Uh oh!
There was an error while loading.Please reload this page.
Merges PR#130427 implementing delattr suggestions by@sobolevn and PR#130455 adding tests for the feature by@Pranjal095
Note: This PR's sole purpose is to combine the feature implementation from PR#130427 with the test cases from PR#130455. All original commits have been preserved to retain proper authorship.