Skip to content

bpo-33569 Preserve type information with InitVar#8927

Merged
ericvsmith merged 7 commits into
python:masterfrom
hackaugusto:initvar_type_information
Jun 3, 2019
Merged

bpo-33569 Preserve type information with InitVar#8927
ericvsmith merged 7 commits into
python:masterfrom
hackaugusto:initvar_type_information

Conversation

@hackaugusto

@hackaugusto hackaugusto commented Aug 25, 2018

Copy link
Copy Markdown
Contributor

@the-knights-who-say-ni

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@ericvsmith

Copy link
Copy Markdown
Member

The failing tests need to be addressed. Also, a news blurb is required.

@hackaugusto

Copy link
Copy Markdown
Contributor Author

@ericvsmith I can't reproduce the build failure, the logs from the build:

$ if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then ./python Tools/scripts/patchcheck.py --travis $TRAVIS_PULL_REQUEST; fi
Getting base branch for PR ... origin/master
Getting the list of files that have been added/changed ... 2 files
Fixing Python file whitespace ... 1 file:
  Lib/dataclasses.py
Fixing C file whitespace ... 0 files
Fixing docs whitespace ... 0 files
Please fix the 1 file(s) with whitespace issues
(on UNIX you can run `make patchcheck` to make the fixes)

The output from my machine:

./python ./Tools/scripts/patchcheck.py
Getting base branch for PR ... origin/master
Getting the list of files that have been added/changed ... fatal: ambiguous argument 'origin/master': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
0 files
Fixing Python file whitespace ... 0 files
Fixing C file whitespace ... 0 files
Fixing docs whitespace ... 0 files
Docs modified ... NO
Misc/ACKS updated ... NO
Misc/NEWS.d updated with `blurb` ... NO
configure regenerated ... not needed
pyconfig.h.in regenerated ... not needed

I assumed it could be something with local branch, which doesn't have an upstream set, so I refetched master and rebased, but the error persist. Any pointers? Thanks

@ericvsmith

Copy link
Copy Markdown
Member

@hackaugusto: I've checked but everything looks okay to me. I can't see what the problem is, unfortunately.

Comment thread Lib/dataclasses.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Found it: this line has 5 space indentation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ohhh, little bastard. thanks, should be fixed now :)

@hackaugusto
hackaugusto force-pushed the initvar_type_information branch from 6136311 to 6426b5e Compare September 10, 2018 18:28
Comment thread Lib/dataclasses.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably have a nicer __repr__ here.

Currently you get:

>>> from dataclasses import InitVar
>>> InitVar[int]
<dataclasses.InitVar object at 0x00A041C0>

I'd suggest something like:

    def __repr__(self):
        return f'dataclasses.InitVar[{self.type}]'

for starters, although typing.ClassVar does something more sophisticated in place of {self.type}. Maybe someday when we import typing from dataclasses we can switch to using their helper function. It's probably worth adding a comment to that effect, too.

@hackaugusto hackaugusto Sep 15, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Using f'dataclasses.InitVar[{self.type}]' would look like:

>>> from dataclasses import InitVar
>>> InitVar[int]
dataclasses.InitVar[<class 'int'>]

So I added f'dataclasses.InitVar[{self.type.__name__}]' instead. Should I write a test for this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If that's what gives you the equivalent of:

>>> import typing
>>> typing.ClassVar[int]
typing.ClassVar[int]

Then I'm all for it.

Yes, we'll need tests for this. Nothing complex, InitVar[int] should be good enough.

@hackaugusto hackaugusto Oct 7, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay, I added a really simple test:

self.assertEqual(InitVar[int].type, int)

Would you like tests using __repr__ and @dataclass?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I apologize for taking forever to get back to you on this.

Could you test the repr of an InitVar? Something like (untested) repr(InitVar[int]) == 'dataclasses.InitVar[int]'?

I promise I'll commit this as soon as you add this change.

@hackaugusto
hackaugusto force-pushed the initvar_type_information branch 2 times, most recently from 7828a4a to 93dd162 Compare September 15, 2018 09:25
dataclassses accepts forward declaration of types. This is achieved by
having a string which represents the type, e.g. `InitVar[int]`.

To properly construct the dataclass attributes these annotations must be
interpreted, which is done by pattern matching for the class name and
doing a dynamic lookup for the the name in the corresponding module.
This dynamic lookup returns the **class** being used in the string, and
not the instance which the string represents. Because of that the
predicate `_is_initvar` must check for both the type and instances of
the type.
@ericvsmith ericvsmith self-assigned this Jun 3, 2019
@ericvsmith
ericvsmith merged commit 01ee12b into python:master Jun 3, 2019
@hackaugusto
hackaugusto deleted the initvar_type_information branch June 3, 2019 12:51
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants