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

pre-commit: Add more ruff rules #59

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

cclauss
Copy link
Collaborator

@cclauss cclauss commented Feb 9, 2024

Description

pre-commit: Add more ruff rules
This sets some upper limits on code complexity to increase the maintainability of the code.

Please include:

  • relevant motivation
  • a summary of the change
  • which issue is fixed.
  • any additional dependencies that are required for this change.

Closes: # (issue)

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b85d900) 80.92% compared to head (68bdd10) 80.92%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #59   +/-   ##
=======================================
  Coverage   80.92%   80.92%           
=======================================
  Files         133      133           
  Lines       29410    29410           
  Branches     5053     5053           
=======================================
  Hits        23799    23799           
  Misses       4742     4742           
  Partials      869      869           

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

Copy link

sonarqubecloud bot commented Feb 9, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@ZeroCool940711
Copy link
Contributor

I think we should remove the docstrings about function complexity, Sonarcloud.io already give us information on that which is better in my opinion and you can check here for more details. When we get code that is too complex on a PR the Sonarcloud.io bot the dashboard seems to notify us about it. The docstring from ruff my stay in the code even after we fix the issue and make things even harder to maintain later on, also, it adds to the number of duplicated lines. The rest of the PR looks good tho and I think it will be a good addition since it will let people know about any issue before they even commit the code to their repo.

@cclauss
Copy link
Collaborator Author

cclauss commented Feb 9, 2024

If a contributor comes to modify one of these functions, the docstrings will alert them before they attempt to make modification. SonarCloud would only warn them after they have already made modifications. The # noqa linter directives on these functions are not self-documenting unless contributors have memorized these directives so the docstrings provide in-situ documentation.

If a tool is asserting that lines in docstrings are duplicate code then that rule should be disabled.

@ZeroCool940711
Copy link
Contributor

If a contributor comes to modify one of these functions, the docstrings will alert them before they attempt to make modification. SonarCloud would only warn them after they have already made modifications. The # noqa linter directives on these functions are not self-documenting unless contributors have memorized these directives so the docstrings provide in-situ documentation.

If a tool is asserting that lines in docstrings are duplicate code then that rule should be disabled.

mmm, you are right, then, I will merge this PR and see if today I can reduce the code complexity on these files and a few others.

@ZeroCool940711 ZeroCool940711 merged commit 9a654a4 into Sygil-Dev:main Feb 9, 2024
11 checks passed
@cclauss cclauss deleted the more-ruff branch February 9, 2024 10:58
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