GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-130428: Add tests for delattr suggestions by Pranjal095 · Pull Request #130455 · python/cpython

Conversation

@Pranjal095

@Pranjal095 Pranjal095 commented Feb 22, 2025 •
edited by picnixz
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Depends on #130427.

With reference to the enhancement proposed in issue #130425, tests have been added to ensure proper functionality of delattr suggestions. To maintain consistency and readability of the code, they mirror the tests for getattr suggestions existing in the modified file (Lib/test/test_traceback.py).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Pranjal095
pythongh-130428: Add tests for delattr suggestions
423d777
@ghost

ghost commented Feb 22, 2025 •
edited by ghost
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.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app

bedevere-app Bot commented Feb 22, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added tests Tests in the Lib/test dir awaiting review labels Feb 22, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Feb 22, 2025
Add tests for delattr and setattr suggestions #130428
Open
@picnixz picnixz added the skip news label Feb 22, 2025
picnixz
picnixz reviewed Feb 22, 2025
View reviewed changes
Comment thread Lib/test/test_traceback.py Outdated
actual = self.get_suggestion(lambda: delattr(obj, 'somethingverywrong'))
self.assertNotIn("blech", actual)

def test_delattr_error_bad_suggestions_do_not_trigger_for_small_names(self):

@picnixz picnixz Feb 22, 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.

This test is the same as for getattr, can't it be refactored so that we use the same fixtures but with different calls to self.get_suggestion?

Sorry, something went wrong.

Uh oh!

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

@Pranjal095 Pranjal095 Feb 22, 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 was actually thinking of doing that, but decided to go with duplication for better readability.
On reconsideration with your review, I believe it is redundant and should be refactored. Will update the PR accordingly.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Pranjal095 and others added 2 commits February 23, 2025 02:00
@Pranjal095
Refactored getattr and delattr tests
813c898
@Pranjal095
Merge branch 'main' into delattr-suggestion-tests
e476168
picnixz
picnixz reviewed Feb 22, 2025
View reviewed changes
Comment thread Lib/test/test_traceback.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Pranjal095
Added else branches to handle unrecognized operations
f980e4f
@tomasr8

tomasr8 commented Feb 23, 2025

Copy link
Copy Markdown
Member

@Pranjal095 Some of the tests you added are failing, could you fix them please?
All reactions

Sorry, something went wrong.

Uh oh!

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

Pranjal095 commented Feb 23, 2025

Copy link
Copy Markdown
Contributor Author

@tomasr8 The reason for the proposed tests failing is that the pull request implementing name suggestions for delattr is yet to be merged into the main branch. Pull request linked here.
Locally, I ran the tests with the pull request changes and they all passed.
tomasr8 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.
@picnixz

picnixz commented Feb 23, 2025

Copy link
Copy Markdown
Member

Let's put it as a draft until #130427 is merged then.
tomasr8 and Pranjal095 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
picnixz marked this pull request as draft February 23, 2025 12:44
@bedevere-app bedevere-app Bot removed the awaiting review label Feb 23, 2025
@encukou

encukou commented Mar 3, 2025

Copy link
Copy Markdown
Member

Usually we'd merge a new feature together with its tests. Could you combine the PRs into one?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Pranjal095 added a commit to Pranjal095/cpython that referenced this pull request Mar 3, 2025
@Pranjal095
Merge remote-tracking branch 'sobolevn/issue-130425' into delattr-sug…
d9ceb27 
…gestions-combined

This merges the delattr suggestion feature implementation by @sobolevn from PR python#130427 with added tests from PR python#130455.
@Pranjal095 Pranjal095 mentioned this pull request Mar 3, 2025
gh-130425: Add "Did you mean [...]" suggestions for del obj.attr #130799
Closed
@Pranjal095

Pranjal095 commented Mar 3, 2025

Copy link
Copy Markdown
Contributor Author

I've made a new PR combining the commits from both corresponding forks. Kindly let me know if there are any further modifications to be made.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz picnixz mentioned this pull request Mar 3, 2025
gh-130425: Add "Did you mean" suggestion for del obj.attr #130427
Closed
@python-cla-bot

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.

CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Pranjal095 Pranjal095 closed this Aug 22, 2025
@Pranjal095
Pranjal095 deleted the delattr-suggestion-tests branch August 22, 2025 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz left review comments

Uh oh!

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

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

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.

4 participants

@Pranjal095 @tomasr8 @picnixz @encukou
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.