Announcing Project Blackjack: hunting for NVDA add-on bugs and coding style improvements with Flake8


 

Hello NVDA add-ons and development community,

 

For those new to the community: I’m Joseph Lee, one of the volunteer code contributors to NVDA screen reader project. I hope everyone is staying safe and healthy during this uncertain time.

 

Like many of you, the ongoing COVID-19 pandemic had a profound impact in my life. After thinking about what to do while juggling online classes, I decided to take a look at the overall health of the NVDA add-ons ecosystem, specifically a way to hunt for bugs and improve code quality and style. The result is Project Blackjack, a set of recommendations for doing just that (linting) with help from Flake8.

 

As some of you might be aware by now, NVDA project is using Flake8, a popular coding style checker (linter) to deal with coding style. This tool allows us (NVDA contributors) to look at potential issues that can arise from inconsistent coding style, and in some cases, find things that appear to be harmless but are actual bugs (syntax warnings and errors and such). Some of you posting pull requests to GitHub may have noticed messages about Flake8 errors on your code, with the tool asking you to fix issues found.

 

In April 2020, I decided to run Flake8 against add-ons I have developed so far. Not only this tool found linting issues, it found potential bugs, and in at least two occasions, actual bugs that weren’t obvious at first. Notable finds include error handling logic error in encoder support in StationPlaylist, unused variable in Resource Monitor, and a need to update Weather app module in Windows 10 App Essentials to modern NVDA coding standards. Thanks to this tool, these three add-ons have gone through coding style changes, bug fixes, and passes Flake8 test.

 

Because linting NVDA source code itself is  something that will take months, I propose that add-ons community use Flake8 to look into coding style, and in some cases, hunt for potential and actual bugs. To do this:

 

  1. Install Flake8. You can do it as a standalone module (pip install flake8), or you can do so when you run “scons source” followed by “scons lint base=origin/master” after cloning NVDA source code. I highly recommend the latter method, as it not only will help you with add-ons, but also useful for people submitting pull requests, as they can use it to catch linting (coding style) issues before you commit changes.
  2. Once Flake8 is installed, switch to your add-on source code directory (root directory if you are using Git).
  3. Run Flake8 (flake8 sourceDir). This will present places where Flake8 found issues.
  4. For a detailed output, ask Flake8 for statistics (flake8 –statistics sourceDir), and you can ask it to print source code as errors are checked (flake8 –show-source sourceDir).
  5. You can also specify Flake8 options via a config file (I recommend using NVDA’s own Flake8 config file as a starting point and customize accordingly).

 

For more info on Flake8, take a look at:

https://flake8.pycqa.org/en/latest/

 

Recommended usage while developing and maintaining add-ons:

 

  1. Install Flake8 and run it against your add-on code (latest version if possible; if you are using Git, try using on master or a branch where you actively develop your add-on).
  2. As Flake8 finds issues, read the source code and think about how to resolve them. Depending on issues Flake8 finds, you can fix it on the spot (arithmetic operator issues (E225, E227, E228), look at where things come from (E403, E405), think about syntax changes (F632, for instance) and such.
  3. After each change, run Flake8 to make sure issues are addressed (no longer appears).
  4. Test your add-on before committing changes (do so while using the add-on yourself), as lint changes may introduce regressions.
  5. After you are satisfied with changes, commit it. You can either do so from your master branch, or if you want, use a separate branch for all Flake8 related work.

 

Tips:

  • Flake8 linting is optional but highly recommended.
  • Don’t make too many changes at once.
  • Always test changes with both Flake8 and by running your add-on.
  • Unless you know what you are doing, split your work over multiple releases.
  • Lint regularly (every release, every three to six months, annually, etc.).

 

For NVDA code contributors: to reduce messages you get about Flake8 when you submit pull requests, I recommend doing a local linting and runtime tests first before committing changes. Not only it can reduce Flake8 messages, it can also reduce diffs and let you see that your changes are fine and readable (no regressions, for example).

 

As a practical demonstration of linting, I will be dedicating upcoming version 20.06 release of my add-ons to Flake8 work. Resource Monitor, StationPlaylist, and Windows 10 App Essentials have gone through linting check in April and they are working as advertised.

 

If you have any questions, feel free to ask. For coding style for NVDA project, refer to NVDA wiki article on coding style and contributing.

Thanks.

Cheers,

Joseph


Doug Lee
 

Not sure what the Flake8 utility notices, but here are a few tips I can offer from my conversion of much Python 2 code to Python 3:

I do the following when scanning Python3 code:

1. Resource management:
* Scan for "open("
* For Popen calls, see if subprocess.run would work as well, and replace Popen with run if so.
* For open() calls, use "with" to put the file and its usage inside a context manager, or minimize how long and how many code lines execute while the file is open. Py3 generates resource warnings when files are not closed when
a process exits, but NVDA doesn't exit often.
* Do the same for Popen/run calls when possible.
* For both, consider whether to use the newline, text, or encoding keyword arguments to ensure consistent data handling.

2. Code complexity:
* Look for "map" calls and see if they can be replaced by comprehensions. The old 2to3 utility does that but sometimes fails and makes a messier line than the original.
* Eliminate filter() calls by using comprehensions with conditionals after the "for" (2to3 also does these).
* (minor) Eliminate extra list() wrappers, but know what you're doing first. Examples: "for e in list(iter)" can just be "for e in iter"; but "print(iter)" works better as "print(list(iter))"
2to3 puts list() around too many things, though of course that's safer than too few.

And one that I bet is rarely needed but that bit me hard twice this week during a roughly 11,000 line 2-to-3 conversion:

3. Look out for the string "cmp" anywhere. __cmp__() and the cmp() function from py2 are gone in py3, and replacing them can take some thinking in complex sorting cases.

On Thu, Apr 30, 2020 at 01:16:46PM -0700, Joseph Lee wrote:
Hello NVDA add-ons and development community,


For those new to the community: I’m Joseph Lee, one of the volunteer
code contributors to NVDA screen reader project. I hope everyone is
staying safe and healthy during this uncertain time.


Like many of you, the ongoing COVID-19 pandemic had a profound impact
in my life. After thinking about what to do while juggling online
classes, I decided to take a look at the overall health of the NVDA
add-ons ecosystem, specifically a way to hunt for bugs and improve code
quality and style. The result is Project Blackjack, a set of
recommendations for doing just that (linting) with help from Flake8.


As some of you might be aware by now, NVDA project is using Flake8, a
popular coding style checker (linter) to deal with coding style. This
tool allows us (NVDA contributors) to look at potential issues that can
arise from inconsistent coding style, and in some cases, find things
that appear to be harmless but are actual bugs (syntax warnings and
errors and such). Some of you posting pull requests to GitHub may have
noticed messages about Flake8 errors on your code, with the tool asking
you to fix issues found.


In April 2020, I decided to run Flake8 against add-ons I have developed
so far. Not only this tool found linting issues, it found potential
bugs, and in at least two occasions, actual bugs that weren’t obvious
at first. Notable finds include error handling logic error in encoder
support in StationPlaylist, unused variable in Resource Monitor, and a
need to update Weather app module in Windows 10 App Essentials to
modern NVDA coding standards. Thanks to this tool, these three add-ons
have gone through coding style changes, bug fixes, and passes Flake8
test.


Because linting NVDA source code itself is something that will take
months, I propose that add-ons community use Flake8 to look into coding
style, and in some cases, hunt for potential and actual bugs. To do
this:


1. Install Flake8. You can do it as a standalone module (pip install
flake8), or you can do so when you run “scons source” followed by
“scons lint base=origin/master” after cloning NVDA source code. I
highly recommend the latter method, as it not only will help you
with add-ons, but also useful for people submitting pull requests,
as they can use it to catch linting (coding style) issues before
you commit changes.
2. Once Flake8 is installed, switch to your add-on source code
directory (root directory if you are using Git).
3. Run Flake8 (flake8 sourceDir). This will present places where
Flake8 found issues.
4. For a detailed output, ask Flake8 for statistics (flake8
–statistics sourceDir), and you can ask it to print source code as
errors are checked (flake8 –show-source sourceDir).
5. You can also specify Flake8 options via a config file (I recommend
using NVDA’s own Flake8 config file as a starting point and
customize accordingly).


For more info on Flake8, take a look at:

[1]https://flake8.pycqa.org/en/latest/


Recommended usage while developing and maintaining add-ons:


1. Install Flake8 and run it against your add-on code (latest version
if possible; if you are using Git, try using on master or a branch
where you actively develop your add-on).
2. As Flake8 finds issues, read the source code and think about how to
resolve them. Depending on issues Flake8 finds, you can fix it on
the spot (arithmetic operator issues (E225, E227, E228), look at
where things come from (E403, E405), think about syntax changes
(F632, for instance) and such.
3. After each change, run Flake8 to make sure issues are addressed (no
longer appears).
4. Test your add-on before committing changes (do so while using the
add-on yourself), as lint changes may introduce regressions.
5. After you are satisfied with changes, commit it. You can either do
so from your master branch, or if you want, use a separate branch
for all Flake8 related work.


Tips:
* Flake8 linting is optional but highly recommended.
* Don’t make too many changes at once.
* Always test changes with both Flake8 and by running your add-on.
* Unless you know what you are doing, split your work over multiple
releases.
* Lint regularly (every release, every three to six months, annually,
etc.).


For NVDA code contributors: to reduce messages you get about Flake8
when you submit pull requests, I recommend doing a local linting and
runtime tests first before committing changes. Not only it can reduce
Flake8 messages, it can also reduce diffs and let you see that your
changes are fine and readable (no regressions, for example).


As a practical demonstration of linting, I will be dedicating upcoming
version 20.06 release of my add-ons to Flake8 work. Resource Monitor,
StationPlaylist, and Windows 10 App Essentials have gone through
linting check in April and they are working as advertised.


If you have any questions, feel free to ask. For coding style for NVDA
project, refer to NVDA wiki article on coding style and contributing.

Thanks.

Cheers,

Joseph



References

1. https://flake8.pycqa.org/en/latest/
2. https://nvda-addons.groups.io/g/nvda-addons/message/12266
3. mailto:nvda-addons@nvda-addons.groups.io?subject=Re: [nvda-addons] Announcing Project Blackjack: hunting for NVDA add-on bugs and coding style improvements with Flake8
4. mailto:joseph.lee22590@gmail.com?subject=Private: Re: [nvda-addons] Announcing Project Blackjack: hunting for NVDA add-on bugs and coding style improvements with Flake8
5. https://groups.io/mt/73384032/739029
6. https://nvda-addons.groups.io/g/nvda-addons/post
7. https://addons.nvda-project.org/
8. https://nvda-addons.groups.io/g/nvda-addons
9. https://nvda-addons.groups.io/g/nvda-addons/editsub/739029
10. mailto:nvda-addons+owner@nvda-addons.groups.io
11. https://nvda-addons.groups.io/g/nvda-addons/leave/defanged

--
Doug Lee dgl@dlee.org http://www.dlee.org
Level Access doug.lee@LevelAccess.com http://www.LevelAccess.com
In healthy competition, the best battles are not for status, but for excellence;
and the battles are not between me and you,
but between you and you, and between me and me. (08/15/2009)


 

Hi all,

I'm bumping this thread for the benefit of those who have joined us recently, as well as to restart Project Blackjack (proposal to lint add-on source code now that we have several repos of community add-ons maintained by various communities). Don't worry about old add-on names.

Thanks.

Cheers,

Joseph