flake8 opportunities

[Saumya-Mishra9129]

@quozl @chimosky Kindly Review. Fixes #58 .

[quozl]

You're changing the files that I said in https://github.com/sugarlabs/turtleart-activity/issues/58 shouldn't be changed because they are maintained elsewhere. Changing them here creates a difference that makes it harder to maintain them in future.

I'd prefer .flake8 not have too many ignores added. E402 substantially changes flake8 processing which is why it is often included. F401 also because imports can have a side-effect without being used. But the new ones you've added seem more related to version-specific deficiencies in flake8.

[Saumya-Mishra9129]

But the new ones you've added seem more related to version-specific deficiencies in flake8.

W503,W504 is version specific. They were not in previous flake8 versions but can be seen in 3.7.9. E741 i.e. ambiguous variable name 'l' unfortunately I dont know what to do in this case it is related to sprites.py which is being used in other files also, and as well as in other activities. If I change it , we will not be able to make sprites.py consistent over activities.

[Saumya-Mishra9129]

Also what can we do F841 as I have seen it in many places ./plugins/rfid/serial/serialposix.py:346:17: F841 local variable 'res' is assigned to but never used ./plugins/audio_sensors/audiograb.py:311:13: F841 local variable 'output' is assigned to but never used ./plugins/audio_sensors/audiograb.py:327:9: F841 local variable 'output' is assigned to but never used ./plugins/audio_sensors/audiograb.py:419:9: F841 local variable 'output' is assigned to but never used ./plugins/audio_sensors/audiograb.py:444:9: F841 local variable 'output' is assigned to but never used ./plugins/audio_sensors/audiograb.py:489:9: F841 local variable 'output' is assigned to but never used ./TurtleArt/taprimitive.py:868:9: F841 local variable 'self' is assigned to but never used ./TurtleArt/tatype.py:62:9: F841 local variable 'self' is assigned to but never used and many more. I have commented some lines also in order to fix this.

[quozl]

Re: E741 for sprites.py, please cherry-pick 354257b, taken from Nutrition activity.

sprites.py is one of those files that was copied and pasted into activities as a quick way to get activities written. So we have almost as many variants of sprites.py as we have activities that use it. We should hope to eventually converge the similar files. Once converged, there is a theoretical reduction of future maintenance.

It is a matter of maintenance priorities. Doing flake8 fixes is of a lower priority than converging similar source code. But that doesn't mean we can't do flake8 fixes because a file is being converged, just that we shouldn't. If we can use a convergence approach to make a flake8 fix, that's best.

[Saumya-Mishra9129]

If we can use a convergence approach to make a flake8 fix, that's best.

I would suggest doing this, as it seems best to me at this time.

Re: E741 for sprites.py, please cherry-pick 354257b, taken from Nutrition activity.

Thanks I will make necessary changes now as suggested by you.

[quozl]

Re: W503. Please cherry-pick 9b2957dc. Method was to (a) remove extra parentheses as they were being used to avoid continuation markers, (b) bring all text onto one line and rerun flake8 to verify W503 is gone, and (c) indent by hand to fit. You will see some strange ways to solve in my demonstration, but from my experience the writers of flake8 have tried hard never to have a situation that is unavoidable.

[Saumya-Mishra9129]

Re: W503. Please cherry-pick 9b2957d. Method was to (a) remove extra parentheses as they were being used to avoid continuation markers, (b) bring all text onto one line and rerun flake8 to verify W503 is gone, and (c) indent by hand to fit. You will see some strange ways to solve in my demonstration, but from my experience the writers of flake8 have tried hard never to have a situation that is unavoidable.

By doing so we will eventually land up in E501 line too long (80 > 79 characters), But Yeah we can solve it :smiley:

[quozl]

Re: E501. Please cherry-pick da23e50. Method was to (a) run autopep8 in aggressive in-place mode, (b) undo some of the damage such as how it splits keyword argument list in BusName.__new__, (c) rerun flake8 and iterate with editor to wrap some of the multi-line comments and other lines that autopep8 would not touch.

Please, please, do not try to fix flake8 warnings and errors by just running autopep8 and committing the result. (a) anyone could do that, we want your brain not your axe, (b) the result is sometimes hard to read, and (c) the changes can be more extensive than needed.

However, autopep8 can be handy to give you some ideas.

Also, a caution that we should never try to fix flake8 warnings in code that doesn't work. It complicates the git blame and history, making git bisect more difficult. This boring flake8 type of maintenance is best done when all problems are solved and the source code looks like being stable for a while ... or where it has been stable for a while and we are about to add a slew of new problems.

[quozl]

Thanks. Good progress. I still don't agree with the changes to the .flake8 file. I think only E402 is needed.

Re: F841 local variable is assigned to but never used. Look at the statement on the line and reverse-engineer the direct and indirect effects. Where there are no indirect effects, the statement can be removed. Where there are indirect effects, such as calls to other functions, the assignment can be removed, leaving just the calls. Then step back and look at the change. Wonder why the functions returned anything at all, and if the program should be doing something with the return.

For example, in audiograb.py there is a repeating pattern of using check_output to capture the stdout stream of a shell command, only to discard it. For many of the commands no output is expected. Also, if the command fails, it might not be important. For example restoring the Analog Mic Boost level using amixer is harmless and useless if the system doesn't have that mixer control.

The name of the function check_output provided by the standard library seems to be a hint that many people call it without checking the output, and it doesn't actually check any output itself.

[Saumya-Mishra9129]

Re: E501. Please cherry-pick da23e50. Method was to (a) run autopep8 in aggressive in-place mode, (b) undo some of the damage such as how it splits keyword argument list in BusName.new, (c) rerun flake8 and iterate with editor to wrap some of the multi-line comments and other lines that autopep8 would not touch. Please, please, do not try to fix flake8 warnings and errors by just running autopep8 and committing the result. (a) anyone could do that, we want your brain not your axe, (b) the result is sometimes hard to read, and (c) the changes can be more extensive than needed. However, autopep8 can be handy to give you some ideas. Also, a caution that we should never try to fix flake8 warnings in code that doesn't work. It complicates the git blame and history, making git bisect more difficult. This boring flake8 type of maintenance is best done when all problems are solved and the source code looks like being stable for a while ... or where it has been stable for a while and we are about to add a slew of new problems.

Thanks for suggestions. I have tried these methods for rest of the files, it worked but yess there are places where E501 can't be fixed, and we will eventually landup with W503 or W504 if we try to fix.(You can look at lines 1286 and 646 in TurtleArtActivity.py and there are many other places also like this). So I added E501 in .flake8 file and fixed W503 and W504. You can review now again.

[Saumya-Mishra9129]

Re: F841 local variable is assigned to but never used. Look at the statement on the line and reverse-engineer the direct and indirect effects. Where there are no indirect effects, the statement can be removed. Where there are indirect effects, such as calls to other functions, the assignment can be removed, leaving just the calls. Then step back and look at the change. Wonder why the functions returned anything at all, and if the program should be doing something with the return. For example, in audiograb.py there is a repeating pattern of using check_output to capture the stdout stream of a shell command, only to discard it. For many of the commands no output is expected. Also, if the command fails, it might not be important. For example restoring the Analog Mic Boost level using amixer is harmless and useless if the system doesn't have that mixer control. The name of the function check_output provided by the standard library seems to be a hint that many people call it without checking the output, and it doesn't actually check any output itself.

Thanks I am yet to apply these changes for F841 fix as it will require some testing as you said. Also there are E741 still exist in tasprite_factory.py which also needs an alternative.

[quozl]

Thanks for suggestions. I have tried these methods for rest of the files, it worked but yess there are places where E501 can't be fixed, and we will eventually landup with W503 or W504 if we try to fix.(You can look at lines 1286 and 646 in TurtleArtActivity.py and there are many other places also like this). So I added E501 in .flake8 file and fixed W503 and W504. You can review now again.

Thanks, but I disagree. It was easily fixed, using one of the techniques that I demonstrated in https://github.com/sugarlabs/turtleart-activity/pull/79/commits/b1983c3f84fa7792ed668d8ea3fc345643d231f3. Please make sure you go through the changes in that commit, reading the diff and understanding what the change was. Please add 418c664e3f1e511746c05863d6a66b86ce413dd2 and continue with the other E501. Don't put E501 in the .flake8 file without finding something that certainly cannot be fixed. I've not been unable to fix E501 in the same way that I was unable to fix E402.

By the way, E402 doesn't seem to happen any longer with flake8 3.7.9, can you confirm? We might aim for an empty .flake8 file.

Thanks I am yet to apply these changes for F841 fix as it will require some testing as you said. Also there are E741 still exist in tasprite_factory.py which also needs an alternative.

Thanks, but I disagree. Some changes for F841 won't require testing. For instance just removing output = in the statement output = check_output() would not require any testing if output is not referenced later in the function. The E741 in tasprite_factory.py also won't need testing, and will be to change l to something that sufficiently communicates the name of the fourth parameter to SVG 2 Paths - elliptical arc curve command "A". I suggest laf.

(Fascinating that the elliptical arc curve commands were added to SVG because they were like the turtle graphics commands ... one of those rare situations in which turtle graphics as an implementation design pattern affected the SVG specification, and around it comes again and an implementation of turtle graphics is able to use the specification.)

[Saumya-Mishra9129]

By the way, E402 doesn't seem to happen any longer with flake8 3.7.9, can you confirm? We might aim for an empty .flake8 file.

No I am still getting E402 with Python3.7 and flake8 3.7.9. Unfortunately we cant aim here. I am done now with my all changes you can review and merge now.

[Saumya-Mishra9129]

@pro-panda @quozl A review is requested once.

[quozl]

Yes, it is on my queue.

[rhl-bthr]

Skipping review since @quozl is taking it up