Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Revert "GH-96145: Add AttrDict to JSON module for use with object_hook (#96146)"#105948

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

Merged
Yhg1s merged 1 commit intopython:mainfromambv:revert-attrdict
Jun 26, 2023

Conversation

ambv
Copy link
Contributor

@ambvambv commentedJun 20, 2023
edited by github-actionsbot
Loading

This reverts commit1f0eafa.

More discussion on the issue.


📚 Documentation preview 📚:https://cpython-previews--105948.org.readthedocs.build/

asottile, jkittner, encukou, erlend-aasland, CAM-Gerlach, and sergeii reacted with thumbs up emoji
Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

I'm not against the concept, it sounds appealing. But it seems like the implementation has some issues and deserves to be discussed. Maybe it was too early to put it in Python 3.12.

erlend-aasland, corona10, CAM-Gerlach, gpshead, deronnax, and sergeii reacted with thumbs up emoji
Copy link
Contributor

@erlend-aaslanderlend-aasland left a comment

Choose a reason for hiding this comment

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

+1 for this revert.

@rhettinger
Copy link
Contributor

rhettinger commentedJun 25, 2023
edited
Loading

the implementation has some issues

Please elaborate. This is the simplest possible implementation that meets the spec of being fully substitutable for a regular dict while allowing optional attribute access for all valid identifiers that aren't keywords or dict methods. The implementation is pretty much unchanged since introduced by Martelli twenty years ago and is the usual recipe that people have been using in practice.

While it is possible to contrive a funky case like Jelle'spop example, this seems tonever arise in practice. People who wrangle data for living will benefit by having this option for JSON's object hook. It greatly improves the readability of data access code in much the same way as dataclasses, namedtuple, Enum, Panda's dataframes, and SimpleNamespace.

I would support marking this asprovisional to leave wiggle room for dealing with Jelle'spop example, but I think the feature should not be reverted. The code is immediately usable and will be of great service to the data wranglers who deal with heavily nested JSON. If you all decide to take this code out, I predict that this will never get done and will be gone forever. Our users won't get to have nice things.

We've already has two decades of people reinventing this for the own projects, so it is not doing to mature much from here. Also, the people like CAM Gerlack who fundamentally oppose this programming practice data will likely never change their minds despite the successes of dataclasses, SimpleNamespace, and argparse.Namespace. ISTM those folks have never personally felt the pain of using chains of square brackets and quotes for lookups into known schemas, nor suffered the consequent readability issues (but acknowledge that I can't really know what is in their minds).

I vote for shipping it as-is, or perhaps marking this a provisional , or perhaps adding a note that the handling ofd.pop and friends is subject to change. In the end, no matter how much discussion you want to have, the core functionality will remain very close to what we have now. Flat-out reverting this is likely to just kill the idea forever (I have lost the spirit and energy for pursuing this further).

czuba reacted with thumbs up emoji

@rhettinger
Copy link
Contributor

rhettinger commentedJun 25, 2023
edited
Loading

@JelleZijlstra Would this minor edit satisfy your concern about thead.pop = 3 corner case?

    def __setattr__(self, attr, value):        if hasattr(dict, attr):            raise AttributeError(f'Cannot set attribute {attr!r}.  Try d[{attr!r}] instead.')        self[attr] = value    def __delattr__(self, attr):        if hasattr(dict, attr):            raise AttributeError(f'Cannot delete attribute {attr!r}.  Try d[{attr!r}] instead.')        try:            del self[attr]        except KeyError:            raise AttributeError(attr) from None

The net result is thatd.<dictmethodname> always means the dictionary method even in the context of setting and deleting. I believe that addresses the only known corner case.

tushar-deepsource and GrigoriiKushnir reacted with thumbs up emoji

@Yhg1s
Copy link
Member

As RM, can I please ask that we not iterate on design right before the final beta release? We'd have one chance to get it exactly right, and regardless of my own opinion on the suitability of the feature, I don't want to deal with the fallout of getting this wrong.

erlend-aasland, AlexWaygood, corona10, gpshead, CAM-Gerlach, gvanrossum, neutrinoceros, and kucera-lukas reacted with thumbs up emoji

@Yhg1sYhg1s merged commitd3af83b intopython:mainJun 26, 2023
@miss-islington
Copy link
Contributor

Thanks@ambv for the PR, and@Yhg1s for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@bedevere-bot
Copy link

GH-106117 is a backport of this pull request to the3.12 branch.

asottile and erlend-aasland reacted with hooray emoji

@bedevere-botbedevere-bot removed the needs backport to 3.12only security fixes labelJun 26, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestJun 26, 2023
…ct_hook (pythonGH-96146)" (pythonGH-105948)This reverts commit1f0eafa.(cherry picked from commitd3af83b)Co-authored-by: Łukasz Langa <lukasz@langa.pl>
Yhg1s pushed a commit that referenced this pull requestJun 26, 2023
…ect_hook (GH-96146)" (GH-105948) (#106117)Revert "GH-96145: Add AttrDict to JSON module for use with object_hook (GH-96146)" (GH-105948)This reverts commit1f0eafa.(cherry picked from commitd3af83b)Co-authored-by: Łukasz Langa <lukasz@langa.pl>
@ambvambv deleted the revert-attrdict branchAugust 21, 2023 11:15
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@brettcannonbrettcannonbrettcannon approved these changes

@gpsheadgpsheadgpshead approved these changes

@vstinnervstinnervstinner approved these changes

@JelleZijlstraJelleZijlstraJelleZijlstra approved these changes

@corona10corona10corona10 approved these changes

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

@CAM-GerlachCAM-GerlachCAM-Gerlach approved these changes

@AlexWaygoodAlexWaygoodAlexWaygood approved these changes

@Yhg1sYhg1sAwaiting requested review from Yhg1s

Assignees
No one assigned
Projects
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

13 participants
@ambv@rhettinger@Yhg1s@miss-islington@bedevere-bot@brettcannon@gpshead@vstinner@JelleZijlstra@corona10@erlend-aasland@CAM-Gerlach@AlexWaygood

[8]ページ先頭

©2009-2025 Movatter.jp