Skip to content

Conversation

@seanbudd
Copy link
Member

@seanbudd seanbudd commented Aug 13, 2025

Link to issue number:

Follow up to #18680

Summary of the issue:

We use a glob import in versionInfo which is not recommended

Description of user facing changes:

Description of developer facing changes:

versionInfo no longer imports all variables from buildVersion

Description of development approach:

Update imports

Testing strategy:

Smoke tests

Known issues with pull request:

None

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.

@coderabbitai summary

Copilot AI review requested due to automatic review settings August 13, 2025 04:20
@seanbudd seanbudd requested a review from a team as a code owner August 13, 2025 04:20
@seanbudd seanbudd requested a review from SaschaCowley August 13, 2025 04:20

This comment was marked as outdated.

@seanbudd seanbudd requested a review from Copilot August 13, 2025 04:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR removes the glob import (star import) from versionInfo.py to follow Python best practices and improve code clarity by explicitly importing only required variables.

  • Replaces from buildVersion import * with explicit imports in versionInfo.py
  • Updates all modules importing version-related variables to use buildVersion instead of versionInfo where appropriate
  • Updates documentation and configuration files to reference the correct module for version information

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
source/versionInfo.py Replaces glob import with explicit imports and updates string formatting
source/updateCheck.py Changes imports from versionInfo to buildVersion for version checking
source/setup.py Moves build-related imports to buildVersion module
source/installer.py Updates version references to use buildVersion instead of versionInfo
source/gui/startupDialogs.py Changes window title to use buildVersion.name
source/gui/init.py Updates frame and system tray icon names to use buildVersion
source/core.py Changes message window name to use buildVersion
source/appModules/nvda.py Updates NVDA menu name to use buildVersion
sconstruct Updates build configuration to use buildVersion variables
projectDocs/dev/developerGuide/developerGuide.md Updates documentation example to use buildVersion
projectDocs/dev/developerGuide/conf.py Updates Sphinx configuration to use buildVersion
appx/sconscript Updates AppX package configuration to use buildVersion

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@seanbudd seanbudd added this to the 2026.1 milestone Aug 13, 2025
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Aug 14, 2025
@seanbudd seanbudd enabled auto-merge (squash) August 15, 2025 06:27
@seanbudd seanbudd disabled auto-merge August 15, 2025 06:44
@seanbudd seanbudd merged commit aa9c078 into master Aug 15, 2025
18 checks passed
@seanbudd seanbudd deleted the remGlobVI branch August 15, 2025 06:44
@LeonarddeR
Copy link
Collaborator

LeonarddeR commented Aug 15, 2025

There are many add-ons that use versionInfo.version_year and similar constants to check compatibility. I see why the glob was removed, but this is a kind of API breakage that we can and should avoid IMO.

OzancanKaratas pushed a commit to OzancanKaratas/nvda that referenced this pull request Aug 17, 2025
Follow up to nvaccess#18680
Summary of the issue:

We use a glob import in versionInfo which is not recommended
Description of user facing changes:
Description of developer facing changes:

versionInfo no longer imports all variables from buildVersion
Description of development approach:

Update imports
@SaschaCowley
Copy link
Member

@LeonarddeR one of my least favourite features of Python is that there is no (good) way of disallowing transitive imports. I do think transitive imports are a useful feature, but I find it deeply irritating that because I import something into my module, it necessarily becomes available for import from my module. It makes problems like this almost unavoidable. I would really like if we could have - and enforce - a single, canonical import path for all public API symbols.

All that being said, while painful I think this code clean up is ultimately positive.

OzancanKaratas pushed a commit to OzancanKaratas/nvda that referenced this pull request Aug 18, 2025
Follow up to nvaccess#18680
Summary of the issue:

We use a glob import in versionInfo which is not recommended
Description of user facing changes:
Description of developer facing changes:

versionInfo no longer imports all variables from buildVersion
Description of development approach:

Update imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-breaking-change conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants