Skip to content

Make Module call the __init__ of NodeNG #1262

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Dec 16, 2021
Merged

Make Module call the __init__ of NodeNG #1262

merged 13 commits into from
Dec 16, 2021

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Nov 19, 2021 •
edited
Loading

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

Found this while working on the end_col_offset issues.

@cdce8p Do you know why we don't do this? Without this col_offset isn't defined on nodes.Module. We could also define it in the __init__ of nodes.Module, but this seem elegant as well?

Type of Changes

Type
✓ 🐛 Bug fix

Related Issue

Closes #1273

@DanielNoord DanielNoord marked this pull request as draft November 19, 2021 16:51
@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Nov 19, 2021 •
edited
Loading

I guess I know why now: it will set node.lineno to None and we make many unguarded calls to node.lineno in pylint. We tend to do int() + node.lineno which shouldn't be allowed if we know that node.lineno can be None.

Problem for the future! Sorry for pinging!

cdce8p
cdce8p reviewed Nov 19, 2021
View reviewed changes
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.

Do you know why we don't do this?

Unfortunately not.

Without this col_offset isn't defined on nodes.Module.

Haven't though about this tbh, but you're right. It should be defined even if None. That's what all other nodes without lineno and col_offset do too: 40d5560

@cdce8p cdce8p mentioned this pull request Nov 22, 2021
Closed
cdce8p
cdce8p reviewed Nov 22, 2021
View reviewed changes
cdce8p
cdce8p reviewed Nov 22, 2021
View reviewed changes
@DanielNoord DanielNoord marked this pull request as ready for review November 22, 2021 15:22
@DanielNoord DanielNoord requested a review from cdce8p November 22, 2021 15:22
DanielNoord
DanielNoord commented Nov 22, 2021
View reviewed changes
DanielNoord
DanielNoord commented Nov 22, 2021
View reviewed changes
@DanielNoord DanielNoord added this to the 2.9.1 milestone Nov 23, 2021
@DanielNoord DanielNoord added Bug 🪳 Enhancement ✨ Improvement to a component and removed task labels Nov 23, 2021
Pierre-Sassoulas
Pierre-Sassoulas approved these changes Dec 15, 2021
View reviewed changes
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.

LGTM, do you want another review @DanielNoord ?

@DanielNoord
Copy link
Collaborator Author

Let's wait for @cdce8p. He reported the initial issue that prompted this PR.

cdce8p
cdce8p requested changes Dec 15, 2021
View reviewed changes
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.

Just some small comments, looks good otherwise.

@cdce8p cdce8p added the pylint-tested PRs that don't cause major regressions with pylint label Dec 15, 2021
@DanielNoord DanielNoord requested a review from cdce8p December 16, 2021 08:40
@cdce8p cdce8p merged commit dddf2da into pylint-dev:main Dec 16, 2021
@DanielNoord DanielNoord deleted the module-init branch December 16, 2021 11:16
tushar-deepsource pushed a commit to tushar-deepsource/astroid that referenced this pull request Dec 20, 2021
@DanielNoord @cdce8p @tushar-deepsource
Make Module call the __init__ of NodeNG (pylint-dev#1262)
a8487b3 
Co-authored-by: Marc Mueller <[email protected]>
@DanielNoord DanielNoord mentioned this pull request Dec 30, 2021
Merged
@DanielNoord DanielNoord mentioned this pull request Jan 14, 2022
Closed
jacobtylerwalls added a commit to jacobtylerwalls/astroid that referenced this pull request Feb 9, 2022
@jacobtylerwalls
Revert "Make Module call the __init__ of NodeNG (pylint-dev#1262)"
9cf7d8f 
This reverts commit dddf2da.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

@cdce8p cdce8p cdce8p approved these changes

Assignees
No one assigned
Labels
Bug 🪳 Enhancement ✨ Improvement to a component pylint-tested PRs that don't cause major regressions with pylint
Projects
None yet
Milestone
2.9.1
Development

Successfully merging this pull request may close these issues.

nodes.Module don't have a end_lineno and end_col_offset
3 participants
@DanielNoord @Pierre-Sassoulas @cdce8p