Fix F841 - Unused variables

[CyrilleB79]

Link to issue number:

None

Summary of the issue:

NVDA's code contains unused variables, with specific directives for the linter to ignore them (#noqa F841)

Description of user facing changes

There is no more #noqa F841 directive in the code.

Description of development approach

Searched for F841 in the code and solved the situations:

1. Generally, removed the unused variable and checked that its presence without any use was not due to another oversight to use it elsewhere
2. 2. Or keep the previously unused variable and use it where it was aimed to (e.g. in NVDAObjects/window/_msOfficeChart.py)
3. In winConsoleHandler.py, renamed consoleEventHookHandles to consoleWinEventHookHandles since the goal clearly seems to empty the list.

Also used a Flake8 report on an older version of NVDA to identify other F841 issues. This way, I have been able to find undefined private variables (i.e. starting with underscore) that are not caught by Ruff's default rule.

Testing strategy:

Manual checks:

• Excel Charts
• Windows console? (TBD)

Known issues with pull request:

In WordDocumentTextInfo._normalizeFormatField, the RUFF linter did not complain that _startOffset and _endOffset were not used, even if there is no #noqa F841 directives on these lines. It's because Ruff's default config exclude private variables (i.e. starting with underscore) from unused variable checking.
I wonder why there is such default config for Ruff and if excluding private variables from the check may prevent us from finding potential unused variable and related issues.

Code Review Checklist:

• Documentation:
  • Change log entry
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• API is compatible with existing add-ons.
• Security precautions taken.

Summary by CodeRabbit

• Bug Fixes

  • Improved parsing within the Doxyfile processing to enhance reliability.
  • Refined description handling in MS Office objects for better accuracy.
  • Fixed candidate description formatting logic in behaviors.
  • Streamlined series name retrieval in MS Office Chart handling for performance.
  • Optimized selection offsets and embedded object labeling in window edits.
  • Simplified field normalization in Winword processing.
  • Enhanced handling of Outlook meeting/appointment creation forms.
  • Improved logic in PowerPoint controller for role assignments and overlap info.

• Refactor

  • Removed unused variable assignments across various modules to streamline code and improve performance.

• Tests

  • Simplified test code in test_winVersion.py by removing unused variable assignments for clarity and maintainability.

[LeonarddeR]

#noqa F841 only remains where necessary.

If there are any remaning variables, could you please underscore prefix them and remove the noqa?

In WordDocumentTextInfo._normalizeFormatField, the RUFF linter did not complain that _startOffset and _endOffset were not used, i.e. there is no #noqa F841 directives on these lines.

If this is an issue that can be isolated pretty easily, it might be worthwhile to report this to Ruff.

[CyrilleB79]

#noqa F841 only remains where necessary.

If there are any remaning variables, could you please underscore prefix them and remove the noqa?

I have removed the last one in commit f2a7f74. Even if it had been introduced on purpose (cc @josephsl), there was no need to define an unused variable in this unit test.

In WordDocumentTextInfo._normalizeFormatField, the RUFF linter did not complain that _startOffset and _endOffset were not used, i.e. there is no #noqa F841 directives on these lines.

If this is an issue that can be isolated pretty easily, it might be worthwhile to report this to Ruff.

I am just realizing that Ruff ignores private variables(i.e. prefixed with underscore) by default, what Flake8 probably did not do. Why is there such default in Ruff? If we stick with this default configuration, we will miss unused private variables (prefixed with underscore), and thus miss any potential issue.

[LeonarddeR]

I am just realizing that Ruff ignores private variables(i.e. prefixed with underscore) by default, what Flake8 probably did not do. Why is there such default in Ruff? If we stick with this default configuration, we will miss unused private variables (prefixed with underscore), and thus miss any potential issue.

I believe they only ignored them for F841. We could fix this by overriding dummy-variable-rgx

[CyrilleB79]

I am just realizing that Ruff ignores private variables(i.e. prefixed with underscore) by default, what Flake8 probably did not do. Why is there such default in Ruff? If we stick with this default configuration, we will miss unused private variables (prefixed with underscore), and thus miss any potential issue.

I believe they only ignored them for F841.

Yes, that's what I was meaning.

We could fix this by overriding dummy-variable-rgx

Of course. But before overriding the default rule, in the first place, I would be curious to know the reason why they ignore private variables.

[LeonarddeR]

I guess this is by design/convension. For example if you want to loop ten times without needing the iteration variable, you can do for _i in range(10) without the linter complaining.

[josephsl]

Hi,

Ah, the nonexistent Windows version variable...

I did it to improve readability. If assigning a dedicated variable impedes readablity, then I guess we should remove it but with justifications.

Thanks.

[CyrilleB79]

Ah, the nonexistent Windows version variable...

I did it to improve readability. If assigning a dedicated variable impedes readablity, then I guess we should remove it but with justifications.

I have finally removed the unused variable (see commit f2a7f74).
To me, the code already seems extra clear and self explaining as is:

• the name test_getWinVerFromNonExistentRelease describes well what the test does
• the comment indicate more details to explain more
• and the member of the enum (winVersion.WIN10_2003) is also very self explaining

@josephsl, which extra explanation do you expect? Something more in the test's code (other comment, etc.)? Or another explanation comment on this page? Please be more specific if needed. Thanks.

Or

The code for this

[CyrilleB79]

I am just realizing that Ruff ignores private variables(i.e. prefixed with underscore) by default, what Flake8 probably did not do. Why is there such default in Ruff? If we stick with this default configuration, we will miss unused private variables (prefixed with underscore), and thus miss any potential issue.

I believe they only ignored them for F841. We could fix this by overriding dummy-variable-rgx

Setting:

[tool.ruff.lint]
# Only ignore variables named "_".
dummy-variable-rgx = "^_$"

I get only one more issue:
source\virtualBuffers\lotusNotes.py:112:6: F811 Redefinition of unused _searchableAttribsForNodeType from line 54

Though, I do not know this file at all, nor how to test it. And I am not able to git blame the changes in this files: I only get the Ruff and line ending reformatting commits (b31c94c and 8fb8ffc).

@seanbudd, without any other hint, I am tempted to leave this file as is, to keep this potential history, due to impossibility of testing.

Or maybe should I use git blame with a specific parameter to ignore these commits? I thought that it was automatically configured in the repo... Could you help me with this?

Also @seanbudd, do you think that I should modify ruff/settings/#lint_dummy-variable-rgx permanently, i.e. in this PR?

I'll switch this PR to Ready to get the required answered.

[coderabbitai]

Walkthrough

The changes primarily involve code simplifications and optimizations by removing unused variables, streamlining function calls, and adjusting control flows within various modules. These modifications enhance code readability and maintainability without introducing new features or altering existing functionality.

Changes

File	Change Summary
site_scons/site_tools/doxygen.py	Removed lineno and next_key variables, adjusted parsing logic.
source/NVDAObjects/IAccessible/msOffice.py	Removed unnecessary rgb variable assignment in _get_description method.
source/NVDAObjects/behaviors.py	Modified getFormattedCandidateDescription to return empty string if numSymbols not equal to 1.
source/NVDAObjects/window/_msOfficeChart.py	Optimized _get_name method by removing redundant function call.
source/NVDAObjects/window/edit.py	Streamlined method calls in _getSelectionOffsets and _getEmbeddedObjectLabel.
source/NVDAObjects/window/winword.py	Modified _normalizeFormatField to directly remove dictionary keys instead of assigning to variables.
source/appModules/nlnotes.py	Removed states variable assignment in chooseNVDAObjectOverlayClasses method.
source/appModules/outlook.py	Adjusted logic in chooseNVDAObjectOverlayClasses for date picker support in Outlook forms.
source/appModules/powerpnt.py	Updated __init__ method in PowerPointController and modified _get__overlapInfo for direct property calculations.
source/brailleDisplayDrivers/papenmeier.py	Removed hwID variable assignment in connectBluetooth method.
source/browseMode.py	Eliminated selectedItemType variable assignment in onTreeChar method.
source/gui/configProfiles.py	Removed wx.BoxSizer creation in __init__ method.
source/gui/installerGui.py	Removed okButton variable assignment in __init__ method.
source/gui/settingsDialogs.py	Removed unnecessary audioBox variable assignment.
source/installer.py	Removed unused detectNVDAExe variable in copyProgramFiles function.
source/speech/manager.py	Removed currentSynth variable assignment in _processSpeechSequence.
source/winConsoleHandler.py	Renamed consoleEventHookHandles to consoleWinEventHookHandles in disconnectConsole.
tests/unit/test_winVersion.py	Removed unused may2020Update variable assignment, directly referenced winVersion.WIN10_2003.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  -- I pushed a fix in commit <commit_id>, please review it.
  -- Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  -- @coderabbitai generate unit testing code for this file.
  -- @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  -- @coderabbitai generate interesting stats about this repository and render them as a table.
  -- @coderabbitai read src/utils.ts and generate unit testing code.
  -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  -- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)

source/appModules/outlook.py (1)

Line range hint 210-210:
Remove unused variable states.

The variable states is declared but not used in the method. Removing it improves code readability and maintainability.

- states = self.states

[seanbudd]

I get only one more issue:
source\virtualBuffers\lotusNotes.py:112:6: F811 Redefinition of unused _searchableAttribsForNodeType from line 54

I think it would be best to leave this. It might be worth adding more thorough comments noting that the second definition is the one used, however we may want to migrate functionality from the first definition.

Or maybe should I use git blame with a specific parameter to ignore these commits? I thought that it was automatically configured in the repo... Could you help me with this?

You may need to add .git-blame-ignore-revs in your local git config, or use the CLI flag

git blame --ignore-revs-file .git-blame-ignore-revs
git config blame.ignoreRevsFile .git-blame-ignore-revs

do you think that I should modify ruff/settings/#lint_dummy-variable-rgx permanently, i.e. in this PR?

I think there are probably cases where we want to follow the magic rules for a dummy variable.
Perhaps this should be considered in a separate PR.
We could make variable names triple-prefixed with underscores dummy variables for example.

[seanbudd]

@CyrilleB79 - do you plan to continue this?

[CyrilleB79]

@seanbudd, finally, it is ready again.