Review global Ruff and Pylint ignore rules

[matt-graham]

In #1158 we added a series of global Ruff and Pylint ignore rules to pyproject.toml

TLOmodel/pyproject.toml

Lines 131 to 137 in e42bd48

	"E712", # comparison to False
	"E713", # use `not in`
	"E714", # use `is not`
	"E721", # do not compare types
	"E731", # do not assign a lambda expression
	"E741", # ambiguous variable name
	"F601", # dictionary literal key repeated

TLOmodel/pyproject.toml

Lines 96 to 103 in e42bd48

	"E0110", # abstract class instantiated
	"E0611", # no-name-in-module
	"E0702", # raising NoneType
	"E1136", # unsubscriptable type
	"E1137", # doesn't support item assignment
	"E1101", # no-member
	"E1120", # no-value-for-parameter
	"E1130", # bad operand type for unary ~

to get the checks to pass without changing the current code. Ideally we should fix the underlying issues where possible, or where we think it's reasonable to violate the rule in a specific case change the global rule to a local # noqa: ... comment in the relevant part of the code.

---

Ruff rule ignores task list

• Fix E712 (comparison to False) violations and disable global ignore
• Fix E713 (test for membership with not in) violations and disable global ignore
• Fix E714 (test for identity with is not) violations and disable global ignore
• Fix E721 (use isinstance to compare types) violations and disable global ignore
• Fix E731 (do not assign lambda expression) violations and disable global ignore
• Fix E741 (ambiguous variable name) violations and disable global ignore
• Fix F601 (dictionary literal key repeated) violations and disable global ignore

Pylint rule ignores task list

• Remove unused code in tests/test_enhanced_lifestyle.py and tests/test_rti.py causing E1120 violations and remove global ignore
• Import DateOffset from public API in tests/test_simplified_births.py to fix E0611 violation and remove global ignore
• Refactor tests/test_healthburden.py to avoid triggering E1101 and remove global ignore
• Add local ignores for E1130 false positives in tests/test_bladder_cancer.py, tests/test_breast_cancer.py and src/tlo/util.py and remove global ignore
• Refactor tests/test_determinism.py to avoid triggering E1120 and remove global ignore
• Refactor src/tlo/methods/contraception.py, src/tlo/methods/healthburden.py and src/tlo/methods/hiv.py:3172 by using inplace variant of pandas function calls to avoid triggering E1136 / E1137 and remove global ignore
• Fix use of raise warnings.warn in src/tlo/methods/scenario_switcher.py and remove global ignore
• Add local ignore for E0110 false positive in src/tlo/analysis/utils.py and remove global ignore
• Add local ignore for E1101 false positive in src/tlo/logging/helpers.py
• Fix E1101 (no-member) violation in src/tlo/test/random_birth.py involving accessing parameters directly as attribute of module (syntax was removed in Remove attribute based access of module parameters #378)

[matt-graham]

Running ruff tests src with all the current global rule ignores in pyproject.toml removed gives the output

warning: Invalid `# noqa` directive on src/tlo/analysis/utils.py:273: expected a comma-separated list of codes (e.g., `# noqa: F401, F841`).
warning: Invalid `# noqa` directive on src/tlo/analysis/utils.py:275: expected a comma-separated list of codes (e.g., `# noqa: F401, F841`).
src/scripts/calibration_analyses/analysis_scripts/analysis_cause_of_death_and_disability_calibrations.py:61:16: E721 Do not compare types, use `isinstance()`
src/scripts/calibration_analyses/analysis_scripts/analysis_cause_of_death_and_disability_calibrations.py:63:16: E721 Do not compare types, use `isinstance()`
src/scripts/data_file_processing/healthsystem/consumables/consumable_resource_analyses_with_lmis/consumables_availability_estimation.py:221:5: F601 [*] Dictionary key literal `'''Atazanavir /Ritonavir (ATV/r), 300+100mg'''` repeated
src/scripts/data_file_processing/healthsystem/consumables/consumable_resource_analyses_with_lmis/consumables_availability_estimation.py:224:5: F601 [*] Dictionary key literal `'Cotrimoxazole, 960 mg'` repeated
src/scripts/maternal_perinatal_analyses/analysis_scripts/maternal_newborn_health_analysis.py:254:16: E741 Ambiguous variable name: `l`
src/scripts/schistosomiasis/schisto_calibration_check.py:31:1: E731 [*] Do not assign a `lambda` expression, use a `def`
src/tlo/analysis/utils.py:273:13: E731 [*] Do not assign a `lambda` expression, use a `def`
src/tlo/analysis/utils.py:275:13: E731 [*] Do not assign a `lambda` expression, use a `def`
src/tlo/methods/bed_days.py:223:16: E721 Do not compare types, use `isinstance()`
src/tlo/methods/bed_days.py:226:36: E721 Do not compare types, use `isinstance()`
src/tlo/methods/causes.py:21:17: E721 Do not compare types, use `isinstance()`
src/tlo/methods/causes.py:29:26: E721 Do not compare types, use `isinstance()`
src/tlo/methods/causes.py:55:20: E721 Do not compare types, use `isinstance()`
src/tlo/methods/healthsystem.py:2112:29: E714 [*] Test for object identity should be `is not`
src/tlo/methods/healthsystem.py:2400:37: E714 [*] Test for object identity should be `is not`
src/tlo/methods/healthsystem.py:2552:29: E714 [*] Test for object identity should be `is not`
src/tlo/methods/hiv.py:2030:17: E713 [*] Test for membership should be `not in`
tests/test_depression.py:67:13: E712 [*] Comparison to `False` should be `cond is False` or `if not cond:`
tests/test_depression.py:68:13: E712 [*] Comparison to `False` should be `cond is False` or `if not cond:`
tests/test_depression.py:69:13: E712 [*] Comparison to `False` should be `cond is False` or `if not cond:`
tests/test_depression.py:70:13: E712 [*] Comparison to `False` should be `cond is False` or `if not cond:`

For all of the currently ignored rules the number of violations is relatively small and are typically confined to one or a few modules and should be quick to fix (ruff in fact reports that 13 of the 21 could potentially be automatically fixed with --fix option). I think it therefore worth fixing all these and disabling the corresponding ignores, as all the rules I think are also sensible in these cases.

EDIT: Moving task list for invidual fixes to initial issue comment.

To keep the changes minimal and easy to review / merge I'll create separate PRs for each of these.

[matt-graham]

To keep the changes minimal and easy to review / merge I'll create separate PRs for each of these.

On a little bit of further thought, creating 7 separate PRs and triggerring multiple Actions workflows is possibly a bad idea and also may be more faff for reviewing so I'll create a PR with a commit per fix.

[matt-graham]

Running pylint src tests with all the current global rules ignores in pyproject.toml removes gives output

************* Module tests.test_enhanced_lifestyle
tests/test_enhanced_lifestyle.py:65:17: E1120: No value for argument 'seed' in function call (no-value-for-parameter)
************* Module tests.test_rti
tests/test_rti.py:441:4: E1120: No value for argument 'seed' in function call (no-value-for-parameter)
************* Module tests.test_simplified_births
tests/test_simplified_births.py:7:0: E0611: No name 'DateOffset' in module 'pandas._libs.tslibs.offsets' (no-name-in-module)
************* Module tests.test_healthburden
tests/test_healthburden.py:151:27: E1101: Instance of 'DiseaseThatCausesA' has no 'persons_affected' member (no-member)
tests/test_healthburden.py:172:27: E1101: Instance of 'DiseaseThatCausesB' has no 'persons_affected' member (no-member)
tests/test_healthburden.py:194:27: E1101: Instance of 'DiseaseThatCausesAandB' has no 'persons_affected' member (no-member)
tests/test_healthburden.py:195:27: E1101: Instance of 'DiseaseThatCausesAandB' has no 'persons_affected' member (no-member)
tests/test_healthburden.py:216:27: E1101: Instance of 'DiseaseThatCausesC' has no 'persons_affected' member (no-member)
************* Module tests.test_bladder_cancer
tests/test_bladder_cancer.py:267:38: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_bladder_cancer.py:267:38: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_bladder_cancer.py:267:38: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_bladder_cancer.py:267:38: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_bladder_cancer.py:267:38: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_bladder_cancer.py:317:38: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_bladder_cancer.py:317:38: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_bladder_cancer.py:317:38: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_bladder_cancer.py:317:38: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_bladder_cancer.py:317:38: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
************* Module tests.test_determinism
tests/test_determinism.py:55:4: E1120: No value for argument 'left' in function call (no-value-for-parameter)
tests/test_determinism.py:55:4: E1120: No value for argument 'right' in function call (no-value-for-parameter)
************* Module tests.test_breast_cancer
tests/test_breast_cancer.py:267:39: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_breast_cancer.py:267:39: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_breast_cancer.py:267:39: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_breast_cancer.py:267:39: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
tests/test_breast_cancer.py:267:39: E1130: bad operand type for unary ~: NoneType (invalid-unary-operand-type)
************* Module src.tlo.util
src/tlo/util.py:206:58: E1130: bad operand type for unary ~: int64 (invalid-unary-operand-type)
************* Module src.tlo.methods.contraception
src/tlo/methods/contraception.py:591:27: E1136: Value 'p_pregnancy_with_contraception_per_month' is unsubscriptable (unsubscriptable-object)
************* Module src.tlo.methods.healthburden
src/tlo/methods/healthburden.py:330:14: E1136: Value 'self.decompose_yll_by_age_and_time(start_date=date_of_death, end_date=min(self.sim.end_date, date_of_birth + pd.DateOffset(years=self.parameters['Age_Limit_For_YLL'])), date_of_birth=date_of_birth)' is unsubscriptable (unsubscriptable-object)
************* Module src.tlo.methods.scenario_switcher
src/tlo/methods/scenario_switcher.py:61:20: E0702: Raising NoneType while only classes or instances are allowed (raising-bad-type)
************* Module src.tlo.methods.hiv
src/tlo/methods/hiv.py:3172:4: E1137: 'x' does not support item assignment (unsupported-assignment-operation)
src/tlo/methods/hiv.py:3172:35: E1136: Value 'x' is unsubscriptable (unsubscriptable-object)
************* Module src.tlo.logging.helpers
src/tlo/logging/helpers.py:29:52: E1101: Instance of 'RootLogger' has no 'loggerDict' member (no-member)
************* Module src.tlo.analysis.utils
src/tlo/analysis/utils.py:99:13: E0110: Abstract class 'ExcelWriter' with abstract methods instantiated (abstract-class-instantiated)
src/tlo/analysis/utils.py:109:4: E1101: Instance of 'ExcelWriter' has no 'save' member; maybe '_save'? (no-member)
************* Module src.tlo.test.random_birth
src/tlo/test/random_birth.py:88:52: E1101: Instance of 'RandomBirth' has no 'pregnancy_probability' member (no-member)

Many of these seem to be false positives. For example the E0110 violation in src/tlo/analysis/utils.py appears to be hitting against pylint-dev/pylint/issues/3060. The E1101 violations in tests/test_healthburden.py I think are also false positives as the relevant member is later explicitly set on the relevant classes, though I think the code could probably still be refactored here to make it clearer and more fault tolerant. The E1130: bad operand type for unary ~: NoneType violations I think are all false positives as in all cases the ~ appears to be being applied to the output of a pandas.isna call which I think should be valid. The various E1136 unscriptable-object violations seem to be hitting against pylint-dev/pylint/issues/3637.

As for Ruff rules, will add a task list for how to deal with each rule in first issue comment.

[matt-graham]

Accidentally pressed close 😬