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

ci: Use pylint to set limits on code complexity #5771

Merged

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Sep 12, 2023

Related Issues

  • fixes #issue-number

Proposed Changes:

By setting upper limits on code complexity we can use the code review process to require contributors who want to increase the limits to justify why increased complexity is required. These can also be metrics for continuous improvement.

[tool.pylint.'DESIGN']
max-args = 37  # Default is 5
max-attributes = 27  # Default is 7
max-branches = 33  # Default is 12
max-locals = 44  # Default is 15
max-statements = 205  # Default is 50

How did you test it?

% pipx run pylint haystack

Notes for the reviewer

Checklist

@cclauss cclauss requested a review from a team as a code owner September 12, 2023 01:05
@cclauss cclauss requested review from masci and removed request for a team September 12, 2023 01:05
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 6153579868

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 48.936%

Totals Coverage Status
Change from base Build 6150513428: 0.0%
Covered Lines: 11844
Relevant Lines: 24203

💛 - Coveralls

@cclauss
Copy link
Contributor Author

cclauss commented Sep 12, 2023

Observations:

  • Four license compliance jobs fail on a pull request that neither adds nor removes dependencies.
  • The release notes job requests that I attach the label 'ignore-for-release-notes' to the PR but I have no right to do so.
  • The pylint job does a full build and install of haystack and all of its dependencies instead of just actions/checkout.
  • The pylint job skips running pylint when its configuration file pyproject.toml is modified.

@silvanocerza silvanocerza added the ignore-for-release-notes PRs with this flag won't be included in the release notes. label Sep 12, 2023
@silvanocerza
Copy link
Contributor

* Four license compliance jobs fail on a pull request that neither adds nor removes dependencies.

They're running cause pyproject.toml has been edited, we don't check whether there are dependencies edits after that. Could be done I guess, not our top priority to be fair.

In any case they're failing when trying to send the event to Datadog. Feel free to ignore it.

* The release notes job requests that I attach the label 'ignore-for-release-notes' to the PR but I have no right to do so.

That's on us, no worries.

* The pylint job does a full build and install of haystack and all of its dependencies instead of just `actions/checkout`.

Nice catch, didn't notice that. If you want to fix that too you're more then welcome. Otherwise I'll take care of it when I get the time. In any case I created issue #5782 to keep track of this.

* The pylint job skips running pylint when its configuration file `pyproject.toml` is modified.

Ah yeah, it should run on pyproject.toml edits too.

I'm merging this PR even with those failing jobs as they're failing for unrelated reasons to what they're testing.

@silvanocerza silvanocerza merged commit 6846448 into deepset-ai:main Sep 12, 2023
51 of 56 checks passed
@cclauss cclauss deleted the pylint-set-limits-on-code-complexity branch September 12, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-for-release-notes PRs with this flag won't be included in the release notes. topic:build/distribution topic:dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants