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

Add rules checking that builtin and custom libraries are imported in alphabetical order. #1088

Merged
merged 4 commits into from
Jun 6, 2024

Conversation

szymonslodkowski
Copy link
Contributor

Add rules if builtin and custom libraries are sorted alphabetically

@bhirsz
Copy link
Member

bhirsz commented Jun 5, 2024

Thanks for the good PR. In a way it implements #107 .
Though there is caveat here: order of the custom imports may be important (and it often is. For example you need to import B before you import A because B keeps variables you need for A). However it should be fine for built in imports. So the solution could be moving those rules (or at least the custom imports one) to community rules (which are disabled by default and can be enabled). What do you think about it?

robocop/checkers/misc.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 97.61905% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.47%. Comparing base (f62508c) to head (e766b9e).
Report is 18 commits behind head on master.

Files Patch % Lines
robocop/checkers/community_rules/misc.py 97.14% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1088      +/-   ##
==========================================
- Coverage   97.02%   95.47%   -1.55%     
==========================================
  Files          37       38       +1     
  Lines        4670     4775     +105     
==========================================
+ Hits         4531     4559      +28     
- Misses        139      216      +77     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@szymonslodkowski
Copy link
Contributor Author

szymonslodkowski commented Jun 5, 2024

@bhirsz thanks for quick review.
So I will leave the rule for builtins in the file I already implemented.
I was not aware about use cases in which order of imports is important - good to know that there are some. So I will move rule checking rest of imports to community rules.
Maybe there is also sense to implement rule verifying order of resources? I could try also to add such rule in this PR. Is it ok?

@bhirsz
Copy link
Member

bhirsz commented Jun 5, 2024

Yeah, feel free to do so - since it will be also custom rule then we can have such rule for resources as well (people can opt in for it).

@szymonslodkowski
Copy link
Contributor Author

Ok, but I have a question about community rules numbering.
In robocop/checkers/community_rules there is only keywords.py with RULE_CATEGORY_ID = "00" I guess these new rules doesn't fit to this file. I would like to create new one, but what should be RULE_CATEGORY_ID and what should be rule_id?

@bhirsz
Copy link
Member

bhirsz commented Jun 5, 2024

You can decide it. But since we started with 00, then next one could be 01. Community rules are prefixed with 1 so final rule will be for example 10101, 10102...

@bhirsz bhirsz merged commit 6d7ee3b into MarketSquare:master Jun 6, 2024
13 checks passed
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.

2 participants