GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-130425: Add "Did you mean [...]" suggestions for `del obj.attr` by Pranjal095 · Pull Request #130799 · python/cpython

Conversation

Pranjal095 commented Mar 3, 2025 •

edited by picnixz

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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.

Issue: Provide name suggestions for failed attribute deletions #130425

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Pranjal095 requested review from markshannon and methane as code owners

March 3, 2025 18:27

bedevere-app Bot added the awaiting review label

Mar 3, 2025

picnixz changed the title ~~Merge PR #130427 and PR #130455 implementing delattr suggestions~~ gh-130425: Add "Did you mean [...]" suggestions for del obj.attr

Mar 3, 2025

bedevere-app Bot mentioned this pull request

Mar 3, 2025

Provide name suggestions for failed attribute deletions #130425

Closed

picnixz mentioned this pull request

Mar 3, 2025

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

Closed

methane approved these changes

Mar 4, 2025

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting review labels

Mar 4, 2025

sobolevn requested changes

Mar 4, 2025

View reviewed changes

sobolevn left a comment

Copy link

Copy Markdown

Member

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 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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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":

sobolevn Mar 4, 2025

Copy link

Copy Markdown

Member

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 add setattr 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:

class GetattrSuggestionsTests(BaseSuggestionsTests, unittest.TestCase):
    def get_suggestion(self, obj, name):
        # get suggestions using `getattr`

class SetattrSuggestionsTests(BaseSuggestionsTests, unittest.TestCase):
    def get_suggestion(self, obj, name):
        # get suggestions using `setattr`

# the same for `delattr`

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Pranjal095 Mar 4, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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__, @property etc.), and I felt it would be better to discuss the appropriate testing mechanism in a separate PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Pranjal095 Mar 4, 2025

Copy link

Copy Markdown

Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Pranjal095 Mar 19, 2025

Copy link

Copy Markdown

Contributor Author

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.

Hello, just a gentle reminder to review the PR changes whenever convenient:) @sobolevn @picnixz

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting changes and removed awaiting merge labels

Mar 4, 2025

bedevere-app Bot commented Mar 4, 2025

Copy link

Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz commented Mar 4, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

I also don't think that merging a fix with tests into one PR is a good idea

I would have actually expected that to be a feature and not a fix actually!

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

I actually always combined my fixes with my tests to check indeed that my fix is correct. Same for features.

encukou reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Pranjal095 requested a review from sobolevn

March 5, 2025 12:39

python-cla-bot Bot commented Apr 18, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

All commit authors signed the Contributor License Agreement.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Pranjal095 requested a review from methane

April 18, 2025 10:20

Pranjal095 commented Jun 15, 2025

Copy link

Copy Markdown

Contributor Author

Hi @sobolevn @picnixz, it has been a while since I addressed the feedback on this PR and noticed this is still open. Would you mind taking another look when you have a moment? Let me know if there's anything further I can do.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz reviewed

Jul 12, 2025

View reviewed changes

picnixz left a comment

Copy link

Copy Markdown

Member

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.

The interface is now a bit too nested. We have many nested classes and this is not the best I think. I suggest the following:

class AttributeSuggestions:
    def get_suggestion(...): ...

class GetattrSuggestions(AttributeSuggestions): ...
class DelattrSuggestions(AttributeSuggestions): ...
class SetattrSuggestions(AttributeSuggestions): ...

And then create the additional classes as you did that inherit from different mixins. But we don't need to touch SuggestionFormattingTestBase for delattr/getattr/hasattr etc. We can just make a separate class. If there is a common interface between SuggestionFormattingTestBase and those classes, make an even top class, say SuggestionFormattingTestBaseParent and SuggestionFormattingTestBase would inherit from it. That way you can share a common interface (I don't know if I'm clear enough).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Pranjal095 requested review from 1st1, AA-Turner, ambv, ammaraskar, hugovk, jaraco, kumaraditya303, lysnikolaou, pablogsal, tomasr8 and willingc as code owners

July 12, 2025 15:04

Pranjal095 requested review from Fidget-Spinner, ZeroIntensity, asvetlov, brandtbucher, carljm, diegorusso, ezio-melotti, iritkatriel, ncoghlan, savannahostrowski, serhiy-storchaka, tiran and vsajip as code owners

July 12, 2025 15:04

sobolevn and others added 10 commits

July 12, 2025 20:36


          pythongh-130425: Add "Did you mean" suggestion for del obj.attr

ee4081f


          Update NEWS

cf9a052


          pythongh-130428: Add tests for delattr suggestions

c369876


          Refactored getattr and delattr tests

04254ad


          Added else branches to handle unrecognized operations

57deaf3


          Refactor getattr and setattr suggestion tests

bbae851


          docs: -I also implies -P (python#131539)

8ba26c4


          pythonGH-134236: make regen-all (pythonGH-134237)

b1210dd


          pythonGH-131798: Optimize away isinstance calls in the JIT (pythonGH-…

60e6243

…134369)


          Refactor getattr and delattr suggestion tests

0ecb6d8

picnixz marked this pull request as draft

July 12, 2025 15:06

bedevere-app Bot removed the awaiting changes label

Jul 12, 2025

Pranjal095 force-pushed the delattr-suggestions-combined branch from 0b58966 to 0ecb6d8 Compare

July 12, 2025 15:06

picnixz commented Jul 12, 2025

Copy link

Copy Markdown

Member

Please avoid force-pushing. I'm going to close this one as many people were notified and suggeset you to create a fresh PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood removed their request for review

July 12, 2025 15:07

picnixz closed this

Jul 12, 2025

Pranjal095 mentioned this pull request

Jul 12, 2025

gh-130425: Add "Did you mean [...]" suggestions for del obj.attr #136588

Merged

Reviewers

picnixz

picnixz left review comments

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

sobolevn

Awaiting requested review from sobolevn

methane

Awaiting requested review from methane

methane is a code owner

tomasr8

Awaiting requested review from tomasr8

pablogsal

Awaiting requested review from pablogsal

ambv

Awaiting requested review from ambv

1st1

Awaiting requested review from 1st1

lysnikolaou

Awaiting requested review from lysnikolaou

ammaraskar

Awaiting requested review from ammaraskar

hugovk

Awaiting requested review from hugovk

kumaraditya303

Awaiting requested review from kumaraditya303

willingc

Awaiting requested review from willingc

AA-Turner

Awaiting requested review from AA-Turner

jaraco

Awaiting requested review from jaraco

sethmlarson

Awaiting requested review from sethmlarson

brettcannon

Awaiting requested review from brettcannon

freakboy3742

Awaiting requested review from freakboy3742

mhsmith

Awaiting requested review from mhsmith

ericsnowcurrently

Awaiting requested review from ericsnowcurrently

erlend-aasland

Awaiting requested review from erlend-aasland

barneygale

Awaiting requested review from barneygale

corona10

Awaiting requested review from corona10

FFY00

Awaiting requested review from FFY00

encukou

Awaiting requested review from encukou

hauntsaninja

Awaiting requested review from hauntsaninja

ethanfurman

Awaiting requested review from ethanfurman

JelleZijlstra

Awaiting requested review from JelleZijlstra

terryjreedy

Awaiting requested review from terryjreedy

pfmoore

Awaiting requested review from pfmoore

pradyunsg

Awaiting requested review from pradyunsg

ericvsmith

Awaiting requested review from ericvsmith

rhettinger

Awaiting requested review from rhettinger

gaogaotiantian

Awaiting requested review from gaogaotiantian

gpshead

Awaiting requested review from gpshead

berkerpeksag

Awaiting requested review from berkerpeksag

cjw296

Awaiting requested review from cjw296

Eclips4

Awaiting requested review from Eclips4

isidentical

Awaiting requested review from isidentical

pganssle

Awaiting requested review from pganssle

abalkin

Awaiting requested review from abalkin

warsaw

Awaiting requested review from warsaw

ncoghlan

Awaiting requested review from ncoghlan

ezio-melotti

Awaiting requested review from ezio-melotti

vsajip

Awaiting requested review from vsajip

tiran

Awaiting requested review from tiran

iritkatriel

Awaiting requested review from iritkatriel

Fidget-Spinner

Awaiting requested review from Fidget-Spinner

ZeroIntensity

Awaiting requested review from ZeroIntensity

serhiy-storchaka

Awaiting requested review from serhiy-storchaka

carljm

Awaiting requested review from carljm

brandtbucher

Awaiting requested review from brandtbucher

savannahostrowski

Awaiting requested review from savannahostrowski

diegorusso

Awaiting requested review from diegorusso

asvetlov

Awaiting requested review from asvetlov

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Pranjal095 commented Mar 3, 2025 • edited by picnixz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

sobolevn Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

Pranjal095 Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pranjal095 Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

Pranjal095 Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented Mar 4, 2025

Uh oh!

picnixz commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

python-cla-bot Bot commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pranjal095 commented Jun 15, 2025

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

picnixz commented Jul 12, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Pranjal095 commented Mar 3, 2025 •

edited by picnixz

Loading

Uh oh!

There was an error while loading. Please reload this page.

Pranjal095 Mar 4, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

picnixz commented Mar 4, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

python-cla-bot Bot commented Apr 18, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.