Skip to content

Create a NamedTuple instance for NamedTuples #1306

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

Closed
wants to merge 2 commits into from

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Dec 20, 2021

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

This is the first step towards some improvements to the NamedTuple brain. One of the first things I thought we should fix is the fact that we currently lose the fact that something is a NamedTuple. We don't add it to the object's bases and they are undistinguishable from other instances.

I'm not sure if adding a new Instance is actually necessary. I thought it might make sense as NamedTuple behaves differently than tuple and can have default values for example. I have created the T(opic): NamedTuple label to identify some issues related to NamedTuple. I thought a new Instance type could be the basis for solving some of these.

Changes have been tested against current latest commit of pytest.

Type of Changes

Type
✨ New feature

Related Issue

@DanielNoord DanielNoord added T: NamedTuple Issues related to NamedTuple pylint-tested PRs that don't cause major regressions with pylint Enhancement ✨ Improvement to a component labels Dec 20, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.10.0 milestone Dec 20, 2021
@cdce8p cdce8p self-requested a review December 29, 2021 12:11
@Pierre-Sassoulas
Copy link
Member

@cdce8p do you think you'll have time to review this ? I see you self requested a review two months ago. I'm trying to release astroid 2.10 :)

Copy link
Contributor

@areveny areveny 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 beginning to understand Astroid brain a lot better after looking at this!

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

Had finally time to look at it. Tbh I'm myself not exactly the "brain" expert. However from what I've gathered so far, I'm not sure this is the best solution to the problem.

I do understand your desire to have NamedTuple / namedtuple at least somewhere which isn't the case today, but it seems strange to add a new instance type just for that. Wouldn't it be much more intuitive to insert a new base which itself has tuple as base? That's at least roughly similar to the pattern we used in the typing brain.

Regarding the remaining changes, I would probably need more time but it looks like there are 3-4 others bundled here as well. That makes it more challenging to review.

Going forward, I would suggest we (1) move the PR back to 2.11, (2) unbundle additional changes, and (3) explore if inserting a dummy base class would work.

@cdce8p cdce8p modified the milestones: 2.10.0, 2.11.0 Feb 27, 2022
@DanielNoord
Copy link
Collaborator Author

I do understand your desire to have NamedTuple / namedtuple at least somewhere which isn't the case today, but it seems strange to add a new instance type just for that. Wouldn't it be much more intuitive to insert a new base which itself has tuple as base? That's at least roughly similar to the pattern we used in the typing brain.

Hmm, I guess this depends on what bases and Instances are supposed to be. Personally I think it makes more sense to create a NamedTuple Instance that can be re-used rather than defining them through f-strings. In my mind it goes like Instance is an instances of a general ClassDef. A sub-class of Instance is thus an instance of a specific type of ClassDef.
But I can see why that might not always be logical.

Regarding the remaining changes, I would probably need more time but it looks like there are 3-4 others bundled here as well. That makes it more challenging to review.

Going forward, I would suggest we (1) move the PR back to 2.11, (2) unbundle additional changes, and (3) explore if inserting a dummy base class would work.

I'll try and separate some of the work here. Some of the typing should be fairly straightforward to extract and then review for example. I'll get back to you!

@DanielNoord DanielNoord marked this pull request as draft July 2, 2022 10:51
@DanielNoord DanielNoord added Work in progress and removed pylint-tested PRs that don't cause major regressions with pylint labels Jul 2, 2022
@Pierre-Sassoulas
Copy link
Member

@DanielNoord can we close this ?

@DanielNoord
Copy link
Collaborator Author

I'll triage this somewhere next week to see if there is anything still relevant in here

@DanielNoord
Copy link
Collaborator Author

Closing!

@DanielNoord DanielNoord deleted the namedtuple branch November 20, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component T: NamedTuple Issues related to NamedTuple Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants