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

Rename checkers/async.py to async_checker.py to import from another module #10060

Merged
merged 3 commits into from
Nov 5, 2024

Conversation

bgb10
Copy link
Contributor

@bgb10 bgb10 commented Nov 1, 2024

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

image
image

Hi. Thank you for maintaining this great library :)
I wanted to make test code for async checkers, but since name of async checker is async.py, AsyncChecker class in this file cannot be imported anywhere. ('async' is keyword!) Indeed, we imported this nowhere before (just registered checker to linter as below), but to write test code for AsyncChecker, file rename is must.

def register(linter: PyLinter) -> None:
    linter.register_checker(AsyncChecker(linter))

After this PR merged, I'll want to make unit test code for AsyncChecker and adding some functionality.

@Pierre-Sassoulas
Copy link
Member

Thank you for opening the PR. Let's merge this but also if you want to add tests for the async checker functional tests already exists and are nicer to use imo. :)

@Pierre-Sassoulas Pierre-Sassoulas added Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news πŸ”‡ This change does not require a changelog entry labels Nov 1, 2024
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Please upload report for BASE (main@15a5ac0). Learn more about missing BASE report.
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #10060   +/-   ##
=======================================
  Coverage        ?   95.80%           
=======================================
  Files           ?      174           
  Lines           ?    18959           
  Branches        ?        0           
=======================================
  Hits            ?    18163           
  Misses          ?      796           
  Partials        ?        0           
Files with missing lines Coverage Ξ”
pylint/checkers/async_checker.py 97.72% <ΓΈ> (ΓΈ)

This comment has been minimized.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

This should probably have a breaking news fragment as it is breaking change.

@jacobtylerwalls
Copy link
Member

Is it breaking if there was no way to import it by name before?

@DanielNoord
Copy link
Collaborator

Can't you use importlib? pylint itself has found a way to use this checker, so downstream users might have as well

@jacobtylerwalls
Copy link
Member

Then does pylint need to change anything?

@jacobtylerwalls jacobtylerwalls removed the Skip news πŸ”‡ This change does not require a changelog entry label Nov 1, 2024
@jacobtylerwalls
Copy link
Member

Ah I see this is mentioned in the body:

Indeed, we imported this nowhere before

Agree a breaking fragment would
Be good.

@bgb10
Copy link
Contributor Author

bgb10 commented Nov 1, 2024

Thanks for review everybody πŸ‘πŸ»
yeah it is possible to use importlib, but I think it would be much better to prevent using filename as same as identifier.
Does this PR would be deferred until milestone 4.0.0?
I want to write test codes for AsyncChecker, so I don't want this PR to be blocker πŸ˜„

@jacobtylerwalls
Copy link
Member

It's not a blocker, 4.0 is the current version under development.

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.

(I approved because I thought like Jacob that it was impossible to import so it did not break anything). Considering Daniel's point, do you mind adding a changelog @bgb10 ?

@bgb10
Copy link
Contributor Author

bgb10 commented Nov 5, 2024

@Pierre-Sassoulas Added :)

Copy link
Contributor

github-actions bot commented Nov 5, 2024

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

This comment was generated for commit 41126dd

@Pierre-Sassoulas Pierre-Sassoulas merged commit 527505e into pylint-dev:main Nov 5, 2024
44 checks passed
@Pierre-Sassoulas
Copy link
Member

Thank you,for noticing and fixing this ! (Autofix of 'noticing' to 'nothing'... mobile is dangerous, huhu)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking changes for 4.0 🦀 Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants