[false-negative] Fix 8947 for consider-using-min/max-builtin

[rhyn0]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Altered checks to account for statements written in the reverse order of an expected if block.

Closes #8947

[rhyn0]

@Pierre-Sassoulas When you have a chance, could you kindly review? False Negative fix for consider-using-min-builtin / consider-using-max-builtin

[codecov]

Codecov Report

Merging #9127 (aa77f4b) into main (e31a155) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9127   +/-   ##
=======================================
  Coverage   95.76%   95.76%           
=======================================
  Files         173      173           
  Lines       18665    18664    -1     
=======================================
  Hits        17874    17874           
+ Misses        791      790    -1     

Files	Coverage Δ
pylint/checkers/refactoring/refactoring_checker.py	98.33% <100.00%> (+0.10%)	⬆️

[Pierre-Sassoulas]

The primer looks pretty good, there's no false positive there. Do you think it's possible to include the examples from the original issue with a cast to float in order to deal withoffset + value on top of the Name / Const ? If it's a lot of work we might merge as is and create another issue but the design / implementation would probably benefit from being able to handle this generically, what do you think ?

[rhyn0]

I think adding in checks for more complex blocks that use a BinOp or Call is doable in this PR. I'll probably have to break the name value getter and comparator into helper functions and then do something about it there. Just worrying about the case of handling something like below

def i_return_float_2(*args):
  return 2.0

value = 1
value3 = 2
if value < i_return_float_2(value3, True, False, 3, 5, 9):
  value = i_return_float_2(value3, True, False, 3, 5, 9)

Which I guess becomes trivial once I know that those nodes are Call and just compare the list of args.

I'll take a crack at it and try

[Pierre-Sassoulas]

I guess a hacky way to make it work would be to call the as_string function on the node to check if the same representation is used, but I don't know the perf implication of doing that (could be good actually, considering everything that need to be taken into account if we handle each node type differentely).

[rhyn0]

Actually kind of like this idea. Might be able to escape the 'isinstance' ladder that is duplicated throughout that function.

[rhyn0]

Added in the original false negatives, and some additional true negatives to the test cases.

The addition of increasing scope of consider-using-max-builtin caused some pylint errors to be raised in other test cases i.e. access_attr_before_def_false_positive.py so added another ignore code for that.

Took @Pierre-Sassoulas idea of using the node.as_string() method to create a common name for complex operands (float(value3)) and then changed the function to only compare string representation instead of sometimes the value for the cases of Const.

Oh and if anyone can help me figure out how to fix the following 2 errors from the pylint pre-commit hook, I can fix the failing CI check that I have

************* Module script.create_contributor_list
script/create_contributor_list.py:7:0: E0401: Unable to import 'contributors_txt' (import-error)
************* Module pylint.reporters.text
pylint/reporters/text.py:232:16: E0401: Unable to import 'colorama' (import-error)

[Pierre-Sassoulas]

You probably need to install the test requirements with pip install -r requirements_test.txt / pip install -r requirements_test_pre_commit.txt

[Pierre-Sassoulas]

This look pretty good already, waiting for CI to be green and for another maintainer review before merging but this is definitely going into 3.1 !

[rhyn0]

You probably need to install the test requirements with pip install -r requirements_test.txt / pip install -r requirements_test_pre_commit.txt

Thanks doing this was able to resolve those issues for me locally, let's hope that passes in CI.

[github-actions]

🤖 Effect of this PR on checked open source code: 🤖

Effect on home-assistant:
The following messages are now emitted:

1. consider-using-max-builtin:
   Consider using 'max_time_between_advertisements = max(max_time_between_advertisements, time_between_advertisements)' instead of unnecessary if block
   https://github.com/home-assistant/core/blob/8a033ee5544f5ed30b67a395525348f76101432d/homeassistant/components/bluetooth/advertisement_tracker.py#L56
2. consider-using-max-builtin:
   Consider using 'when = max(when, current_time)' instead of unnecessary if block
   https://github.com/home-assistant/core/blob/8a033ee5544f5ed30b67a395525348f76101432d/homeassistant/components/trafikverket_ferry/coordinator.py#L80
3. consider-using-max-builtin:
   Consider using 'max_temp = max(max_temp, min_temp)' instead of unnecessary if block
   https://github.com/home-assistant/core/blob/8a033ee5544f5ed30b67a395525348f76101432d/homeassistant/components/homekit/type_thermostats.py#L830

Effect on music21:
The following messages are now emitted:

1. consider-using-min-builtin:
   Consider using 'leastQuarterLength = min(leastQuarterLength, quarterLength)' instead of unnecessary if block
   https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/voiceLeading.py#L1559
2. consider-using-max-builtin:
   Consider using 'longestQuarterLength = max(longestQuarterLength, quarterLength)' instead of unnecessary if block
   https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/voiceLeading.py#L1579
3. consider-using-max-builtin:
   Consider using 'found = max(found, len(g))' instead of unnecessary if block
   https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/features/jSymbolic.py#L3019
4. consider-using-max-builtin:
   Consider using 'maxStaves = max(maxStaves, staves)' instead of unnecessary if block
   https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/musicxml/xmlToM21.py#L1970
5. consider-using-max-builtin:
   Consider using 'highestTime = max(highestTime, ht)' instead of unnecessary if block
   https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/musicxml/m21ToXml.py#L1554
6. consider-using-max-builtin:
   Consider using 'maxSpines = max(maxSpines, numSpines)' instead of unnecessary if block
   https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/humdrum/spineParser.py#L404
7. consider-using-max-builtin:
   Consider using 'highestTimeInsert = max(highestTimeInsert, o + qL)' instead of unnecessary if block
   https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/stream/base.py#L2841
8. consider-using-max-builtin:
   Consider using 'highestTimeSoFar = max(highestTimeSoFar, candidateOffset)' instead of unnecessary if block
   https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/stream/base.py#L8446
9. consider-using-max-builtin:
   Consider using 'maxVoiceCount = max(maxVoiceCount, len(group))' instead of unnecessary if block
   https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/stream/base.py#L11213
10. consider-using-max-builtin:
    Consider using 'oMax = max(oMax, refStreamHighestTime)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/stream/makeNotation.py#L526
11. consider-using-max-builtin:
    Consider using 'maxLineLength = max(maxLineLength, lineLength)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/braille/text.py#L339
12. consider-using-max-builtin:
    Consider using 'highestUsedLocation = max(highestUsedLocation, textLocation)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/braille/text.py#L572
13. consider-using-min-builtin:
    Consider using 'x = min(x, degree)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/scale/intervalNetwork.py#L831
14. consider-using-max-builtin:
    Consider using 'x = max(x, degree)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/scale/intervalNetwork.py#L850
15. consider-using-max-builtin:
    Consider using 'x = max(x, degree)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/scale/intervalNetwork.py#L873
16. consider-using-max-builtin:
    Consider using 'maxLength = max(maxLength, len(fs))' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/figuredBass/realizer.py#L191
17. consider-using-max-builtin:
    Consider using 'endTimeLow = max(endTimeLow, endTimeLow)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/tree/node.py#L276
18. consider-using-max-builtin:
    Consider using 'endTimeHigh = max(endTimeHigh, endTimeHigh)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/tree/node.py#L278
19. consider-using-max-builtin:
    Consider using 'endTimeLow = max(endTimeLow, endTimeLow)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/tree/node.py#L284
20. consider-using-max-builtin:
    Consider using 'endTimeHigh = max(endTimeHigh, endTimeHigh)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/tree/node.py#L286
21. consider-using-max-builtin:
    Consider using 'endTimeLow = max(endTimeLow, endTimeLow)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/tree/node.py#L518
22. consider-using-max-builtin:
    Consider using 'endTimeHigh = max(endTimeHigh, endTimeHigh)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/tree/node.py#L520
23. consider-using-max-builtin:
    Consider using 'endTimeLow = max(endTimeLow, endTimeLow)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/tree/node.py#L526
24. consider-using-max-builtin:
    Consider using 'endTimeHigh = max(endTimeHigh, endTimeHigh)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/tree/node.py#L528
25. consider-using-min-builtin:
    Consider using 'numChords = min(numChords, len(chordWeights))' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/analysis/reduceChordsOld.py#L89
26. consider-using-max-builtin:
    Consider using 'offset = max(offset, endTime)' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/analysis/reduceChords.py#L387
27. consider-using-min-builtin:
    Consider using 'maximumNumberOfChords = min(maximumNumberOfChords, len(chordWeights))' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/analysis/reduceChords.py#L545
28. consider-using-min-builtin:
    Consider using 'end = min(end, len(self._windowedStream))' instead of unnecessary if block
    https://github.com/cuthbertLab/music21/blob/b6af0b576aa6bfb458fa754879a1737de67f49dc/music21/analysis/windowed.py#L198

Effect on django:
The following messages are now emitted:

1. consider-using-min-builtin:
   Consider using 'MinX = min(MinX, minx)' instead of unnecessary if block
   https://github.com/django/django/blob/e47298aec4fa04416e7082331fbd44bd9f2662aa/django/contrib/gis/gdal/envelope.py#L130
2. consider-using-min-builtin:
   Consider using 'MinY = min(MinY, miny)' instead of unnecessary if block
   https://github.com/django/django/blob/e47298aec4fa04416e7082331fbd44bd9f2662aa/django/contrib/gis/gdal/envelope.py#L132
3. consider-using-max-builtin:
   Consider using 'MaxX = max(MaxX, maxx)' instead of unnecessary if block
   https://github.com/django/django/blob/e47298aec4fa04416e7082331fbd44bd9f2662aa/django/contrib/gis/gdal/envelope.py#L134
4. consider-using-max-builtin:
   Consider using 'MaxY = max(MaxY, maxy)' instead of unnecessary if block
   https://github.com/django/django/blob/e47298aec4fa04416e7082331fbd44bd9f2662aa/django/contrib/gis/gdal/envelope.py#L136

Effect on pandas:
The following messages are now emitted:

1. consider-using-max-builtin:
   Consider using 'dtype = max(dtype, np.dtype('m8[s]'))' instead of unnecessary if block
   https://github.com/pandas-dev/pandas/blob/a76b3f4c73a351a11d2cae6236773192e630d4f5/pandas/tests/arithmetic/test_numeric.py#L267
2. consider-using-max-builtin:
   Consider using 'dtype = max(dtype, mst)' instead of unnecessary if block
   https://github.com/pandas-dev/pandas/blob/a76b3f4c73a351a11d2cae6236773192e630d4f5/pandas/core/dtypes/cast.py#L687
3. consider-using-max-builtin:
   Consider using 'dtype = max(dtype, mst)' instead of unnecessary if block
   https://github.com/pandas-dev/pandas/blob/a76b3f4c73a351a11d2cae6236773192e630d4f5/pandas/core/dtypes/cast.py#L722
4. consider-using-max-builtin:
   Consider using 'max_row_len = max(max_row_len, len(table_row))' instead of unnecessary if block
   https://github.com/pandas-dev/pandas/blob/a76b3f4c73a351a11d2cae6236773192e630d4f5/pandas/io/excel/_odfreader.py#L149

This comment was generated for commit aa77f4b

[Pierre-Sassoulas]

We're going to release 3.1.0 this week-end let's merge 😄

[Pierre-Sassoulas]

Congratulation on becoming a pylint contributor 🎉