Closed anis-campos closed 8 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
bb1a8de
) 93.98% compared to head (46cf450
) 98.02%. Report is 6 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
the current implementation provokes recursion errors because of the VariablesChecker.add_message patch not working properly.
Would you mind creating a test, that demonstrates the issue?
the current implementation provokes recursion errors because of the VariablesChecker.add_message patch not working properly.
Would you mind creating a test, that demonstrates the issue?
I would love to, but i'm not sure how.
I dived on this a bit, turns out the issue is invisible in the tests suits because we are running mono process checks. In our project, we use pylint -j 0
that will run the checks in several threads, one per cpu cores.
This means that I'm actually fixing a thread safety issue.
Now, that being said, I would like to reproduce it here, but I'm not sure how to make a test suit that will use a the -j
option of pylint, as we are not actually running pylint, just unit checking the checkers.
Should we include a new test folder, that would actually run pylint
, something akin to a integration_tests
?
I went with a very simple integration test. This run https://github.com/pylint-dev/pylint-pytest/actions/runs/7310392985 shows the error before the fix
@stdedos , happy new year 🎊. Can we go forward with this PR ?
Hello @anis-campos! Happy new year 🎊🎊🎊
Apologies for the long radio silence. Winter vacations were not ... so much vacations for me, sadly 😅
Thank you for adding the test. While I don't understand "exactly" what is the issue - it is clearly an issue, and one your commit deals with!
The b52f6f6
(#29) commit is also definitely wanted. I'd stamp it immediately, if it were separate.
AFAIS, the essence of your first commit is:
pylint_pytest/__init__.py
register
explicit, rather than implicit/recursive into explicit. 👍 / 👎
There are both arguments here. I'd say it's nicer not to have to remember to register your checker (ofc, "how many times are you going to add new checkers?" is also a question ...)remove_original_variables_checker()
. If we have to do it, we do it 👍 pylint_pytest/checkers/variables.py
patch_add_message
logic into the new CustomVariablesChecker
. 👍 pylint_pytest/checkers/fixture.py
def open(self)
fiddling?? 👍 👍 👍 (You should then be able to remove replacement_add_message
? I think it's unused at this point. What about self.checker.open()
calls?)FixtureChecker
's "private" class variables into public.
I assume you need to access FixtureChecker.pytest_fixtures
from CustomVariablesChecker
without pylint complaining?
If we have to do it, we do it 👍. I'd like to avoid "exposing internals" if not necessary, though 👎 Spraying with # pylint: disable=protected-access
is acceptable (I'm thinking to experiment with exposing less at a later time)There are both arguments here. I'd say it's nicer not to have to remember to register your checker (ofc, "how many times are you going to add new checkers?" is also a question ...)
that was indeed my reasoning, I wasn't sure it it was worth it trying to understand the auto discovery when there is only 3 classes to register.
If we have to do it, we do it 👍. I'd like to avoid "exposing internals" if not necessary, though 👎 Spraying with # pylint: disable=protected-access is acceptable (I'm thinking to experiment with exposing less at a later time)
Hum, it is still a internal use only, so yes, we can keep it private.
the current implementation provokes recursion errors because of the VariablesChecker.add_message patch not working properly. This rework fix the issue by replacing the original variable checker by a subclass, without patch.