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

Refactors related to separating ast creating and linting #7286

Merged
merged 5 commits into from
Aug 14, 2022

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

Refs #7263.

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Aug 10, 2022
@DanielNoord DanielNoord added the Skip news πŸ”‡ This change does not require a changelog entry label Aug 10, 2022
@coveralls
Copy link

coveralls commented Aug 10, 2022

Pull Request Test Coverage Report for Build 2856995021

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 95.245%

Totals Coverage Status
Change from base Build 2852974969: 0.002%
Covered Lines: 16844
Relevant Lines: 17685

πŸ’› - Coveralls

@github-actions

This comment has been minimized.

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.

This would require another refactor but wouldn't it be a good thing to create Γ  'FileGatherer' class to have something testable and decoupled from the configuration in order to decrease the responsabilities of the PyLinter class ?

self.initialize()

# 2) Gather all files
Copy link
Member

Choose a reason for hiding this comment

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

We could create a _gather_all_files fonction and remove comments.

pylint/lint/pylinter.py Outdated Show resolved Hide resolved
@@ -639,23 +645,30 @@ def check(self, files_or_modules: Sequence[str] | str) -> None:
"Missing filename required for --from-stdin"
)

filepath = files_or_modules[0]
# 3) Get all FileItems
Copy link
Member

Choose a reason for hiding this comment

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

Same with _get_all_fileitems.

Co-authored-by: Pierre Sassoulas <[email protected]>
@DanielNoord
Copy link
Collaborator Author

This would require another refactor but wouldn't it be a good thing to create Γ  'FileGatherer' class to have something testable and decoupled from the configuration in order to decrease the responsabilities of the PyLinter class ?

Wouldn't that be a bit overkill? I think it makes more sense to focus on creating a FileChecker which is a step somewhere in this refactoring process πŸ˜„

@github-actions

This comment has been minimized.

@DanielNoord DanielNoord mentioned this pull request Aug 10, 2022
2 tasks
@Pierre-Sassoulas
Copy link
Member

I think it makes more sense to focus on creating a FileChecker which is a step somewhere in this refactoring process

I'm not opinionated about creating a particular class or about a particular design, you know this part of the code better than me so you'll know better than me what to do :)

But I think we need to be able to test those changes at some point, right now we need to test through the PyLinter which means it's hard and we don't. The core internal of pylint (like pylint.message, and some part of the PyLinter) should be unitary tested (well, unitary testable is a first step) .

@DanielNoord
Copy link
Collaborator Author

I'm not opinionated about creating a particular class or about a particular design, you know this part of the code better than me so you'll know better than me what to do :)

That's not necessarily true πŸ˜…

But I think we need to be able to test those changes at some point, right now we need to test through the PyLinter which means it's hard and we don't. The core internal of pylint (like pylint.message, and some part of the PyLinter) should be unitary tested (well, unitary testable is a first step) .

Yeah agreed. For now I'd say let's merge this and #7288. That would definelty set us up for the creation of FileChecker or something like that.

@Pierre-Sassoulas
Copy link
Member

Yeah agreed. For now I'd say let's merge this and #7288. That would definelty set us up for the creation of FileChecker or something like that.

πŸ‘

@DanielNoord DanielNoord enabled auto-merge (squash) August 14, 2022 20:01
@DanielNoord DanielNoord merged commit f8286ac into pylint-dev:main Aug 14, 2022
@DanielNoord DanielNoord deleted the lint-discovery branch August 14, 2022 20:16
@github-actions
Copy link
Contributor

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit b7fc900

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news πŸ”‡ This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants