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

Parse default values in dataclass attributes correctly #1771

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

DanielNoord
Copy link
Collaborator

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

Closes pylint-dev/pylint#7425

Type of Changes

Type
🐛 Bug fix

My god.. This PR...

Our tests for dataclasses aren't really good it seems. Oh well, these PRs should help with that as well.

Refs. #1768 #1764 #1770

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3001456738

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 92.403%

Totals Coverage Status
Change from base Build 2999483585: 0.002%
Covered Lines: 9755
Relevant Lines: 10557

💛 - Coveralls

@altendky
Copy link

altendky commented Sep 6, 2022

Passed CI for me again over at https://github.com/Chia-Network/chia-blockchain/runs/8210700017?check_suite_focus=true. Thanks so much.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank for the quick fix @DanielNoord, that's amazing. Maybe we need 'crash' primers for astroid, this is getting stressful 😄

@Pierre-Sassoulas Pierre-Sassoulas merged commit 3331d62 into pylint-dev:main Sep 6, 2022
@DanielNoord DanielNoord deleted the regression branch September 6, 2022 15:57
@DanielNoord
Copy link
Collaborator Author

Thank for the quick fix @DanielNoord, that's amazing. Maybe we need 'crash' primers for astroid, this is getting stressful 😄

Yeah.. Although I tested against pylint and I don't think this failed. The initial PR was fully tested on the pylint test suite.

@Pierre-Sassoulas
Copy link
Member

Do you mean even the pylint primer ? You need to create your own pylint branch in your private repository and launch the test with a modified astroid requirements for this, right ?

@DanielNoord
Copy link
Collaborator Author

Do you mean even the pylint primer ? You need to create your own pylint branch in your private repository and launch the test with a modified astroid requirements for this, right ?

I checked the branch out locally and ran the pylint test suite, which worked.

I'm planning on doing the same for every PR automatically in astroid, but never got around to implementing it.

We could also start running the primer but that is even more complicated as there are many moving pieces to consider then.

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Sep 6, 2022

I checked the branch out locally and ran the pylint test suite, which worked.

I'm doing that too when I'm suspicious of a PR, but it's very manual.

We could also start running the primer but that is even more complicated as there are many moving pieces to consider then.

I did not put a lot of thought into it but maybe we could be using the pylint's primer json and create an astroid ast for each of the project. It might be the right time to extract pylint's testutil to another repository.

@DanielNoord
Copy link
Collaborator Author

I'm doing that too when I'm suspicious of a PR, but it's very manual.

Should be relatively straightforward though in a workflow. Only need to handle crashes which I haven't thought about fully.

I did not put a lot of thought into it but maybe we could be using the pylint's primer json and create an astroid ast for each of the project. It might be the right time to extract pylint's testutil to another repository.

The main issue is that the whole primer package is geared towards running from the pylint repo. At some point I want to make a more general package to also be able to use it in pydocstringformatter, but so much other stuff to do..

@Pierre-Sassoulas
Copy link
Member

At some point I want to make a more general package to also be able to use it in pydocstringformatter

That does sounds great.

but so much other stuff to do..

Yeap. Maybe hotfixing astroid will not be one of those stuff anymore when we do 😄 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pylint complaint about missing dataclass constructor parameters with new astroid 2.12.6+
4 participants