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

Don't reset similarities config to defaults in parallel mode #4178

Closed
wants to merge 1 commit into from

Conversation

shvenkat
Copy link
Contributor

@shvenkat shvenkat commented Mar 3, 2021

When parallel mode is used (--jobs 0, --jobs 2, ...), the similarities checker seems to ignore the min-similarity-lines config values provided by the user. Instead it seems to use the builtin default value of 4. In "serial mode" (i.e. not specifying --jobs), this is not an issue.

The reduce_map_data method is a class method, so a new instance of the similarity checker is created. I narrowed down the issue to the fact that this instance is not configured. I'm not sure if this is the right way to fix this, but using the originally configured instance of this checker, instead of creating a new one, works.

Steps

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Description

Type of Changes

Type
🐛 Bug fix

Related Issue

When parallel mode is used (`--jobs 0`, `--jobs 2`, ...), the `similarities` checker seems to ignore the `min-similarity-lines` config values provided by the user. Instead it seems to use the builtin default value of 4. In "serial mode" (i.e. not specifying `--jobs`), this is not an issue.

The `reduce_map_data` method is a class method, so a new instance of the similarity checker is created. I narrowed down the issue to the fact that this instance is not configured. I'm not sure if this is the right way to fix this, but using the originally configured instance of this checker, instead of creating a new one, works.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 91.465% when pulling 1b1d6eb on shvenkat:patch-2 into 25e63b0 on PyCQA:master.

@shvenkat shvenkat changed the title Don't reset config to defaults in parallel mode Don't reset similarities config to defaults in parallel mode Mar 3, 2021
@shvenkat
Copy link
Contributor Author

shvenkat commented Mar 4, 2021

Related to #4179. Redundant with #4175.

Copy link
Contributor

@doublethefish doublethefish left a comment

Choose a reason for hiding this comment

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

I don't think this is the whole fix, although the similarities config will be set, the final code-quality report won't take the similarities score into account properly.

Overall, I feel this patch should be rejected, unfortunately.

recombined = SimilarChecker(linter)
try:
recombined = linter._checkers["similarities"][0]
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

Bare except.

Try running the following inside your checkout.
tox -e formatting

@@ -427,7 +427,10 @@ def reduce_map_data(cls, linter, data):
"""Reduces and recombines data into a format that we can report on

The partner function of get_map_data()"""
recombined = SimilarChecker(linter)
try:
recombined = linter._checkers["similarities"][0]
Copy link
Contributor

Choose a reason for hiding this comment

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

As suggested in #4175 a better way of doing this might be to change this method from a @classmethod to an instance method. The call-site supports this.

@DanielNoord
Copy link
Collaborator

Going to close this as the PR that would supersede this has been merged already. The associated issue (#4118) has also been closed.

@DanielNoord DanielNoord closed this Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants