Skip to content
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

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

Merged
merged 1 commit into from Jun 26, 2023

Conversation

ambv
Copy link
Contributor

@ambv ambv commented Jun 20, 2023

This reverts commit 1f0eafa.

More discussion on the issue.


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

Copy link
Member

@vstinner vstinner 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.

Copy link
Contributor

@erlend-aasland erlend-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 commented Jun 25, 2023

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's pop example, this seems to never 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 as provisional to leave wiggle room for dealing with Jelle's pop 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 of d.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).

@rhettinger
Copy link
Contributor

rhettinger commented Jun 25, 2023

@JelleZijlstra Would this minor edit satisfy your concern about the ad.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 that d.<dictmethodname> always means the dictionary method even in the context of setting and deleting. I believe that addresses the only known corner case.

@Yhg1s
Copy link
Member

Yhg1s commented Jun 26, 2023

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.

@ambv ambv removed the DO-NOT-MERGE label Jun 26, 2023
@Yhg1s Yhg1s merged commit d3af83b into python:main Jun 26, 2023
27 of 29 checks passed
@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 the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Jun 26, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 26, 2023
…ct_hook (pythonGH-96146)" (pythonGH-105948)

This reverts commit 1f0eafa.
(cherry picked from commit d3af83b)

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
Yhg1s pushed a commit that referenced this pull request Jun 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 commit 1f0eafa.
(cherry picked from commit d3af83b)

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
@ambv ambv deleted the revert-attrdict branch August 21, 2023 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet