Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend the life of deprecated aliases #13791

Merged
merged 4 commits into from
Jun 14, 2022
Merged

Extend the life of deprecated aliases #13791

merged 4 commits into from
Jun 14, 2022

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Jun 14, 2022

Link to issue number:

None

Summary of the issue:

For deprecated aliases, there is no need to remove them, as they have a minimum maintenance burden.
Add-on authors and contributors have requested we keep aliases where possible.

However, without marking code for removal, it can be hard to find deprecations.
Core contributors and add-on authors may want to avoid deprecated APIs.
As such, a way to test the deprecated API being removed is needed.

Description of how this pull request fixes the issue:

Extends the life of all currently deprecated aliases.
Warnings will be logged when attempting to use deprecated aliases (except for controlTypes, due to the noise of this).
Adds a global variable to mark code as deprecated, which allows developers to test NVDA with deprecated APIs removed.

As a result, there is currently no API breakages staged for 2023.1

Testing strategy:

Manual testing:

Test Steps 1:

  1. Run NVDA and open the Python console.
  2. Try importing deprecated aliases, ensure they are imported successfully but a warning is logged:
    • import appModules.calculatorapp
    • from gui import quit; quit() (calling quit should trigger the log warning)
    • from appModuleHandler import NVDAProcessID
    • from easeOfAccess import ROOT_KEY

Test Steps 2:

  1. From NVDA source, change globalVars._allowDeprecatedAPI = False
  2. Run NVDA from source and open the Python console.
  3. Try importing deprecated aliases, ensure they all raise a ModuleNotFoundError/ImportError:
    • import appModules.calculatorapp
    • from gui import quit
    • from appModuleHandler import NVDAProcessID
    • from easeOfAccess import ROOT_KEY

Known issues with pull request:

None

Change log entries:

Included in PR

Code Review Checklist:

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

@seanbudd seanbudd marked this pull request as ready for review June 14, 2022 01:20
@seanbudd seanbudd requested a review from a team as a code owner June 14, 2022 01:20
@seanbudd seanbudd requested review from feerrenrut and removed request for a team June 14, 2022 01:20
@AppVeyorBot
Copy link

See test results for failed build of commit 70f38a5108

source/appModuleHandler.py Outdated Show resolved Hide resolved
source/globalVars.py Outdated Show resolved Hide resolved
source/config/__init__.py Outdated Show resolved Hide resolved
source/appModuleHandler.py Outdated Show resolved Hide resolved
@seanbudd seanbudd requested a review from feerrenrut June 14, 2022 02:59
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Looks good, assuming you've tested that these new warning show in the log correctly.
Please also update the description to note that warnings will be logged for usage of deprecations.

@seanbudd
Copy link
Member Author

I can confirm the testing of warning logs.
I've updated the testing steps and description.
Note that controlTypes do not log warnings, as it would be too noisy.

@AppVeyorBot

This comment was marked as off-topic.

@seanbudd seanbudd merged commit d3712e5 into beta Jun 14, 2022
@seanbudd seanbudd deleted the extendDeprecatedAliases branch June 14, 2022 05:28
@nvaccessAuto nvaccessAuto added this to the 2022.3 milestone Jun 14, 2022
@CyrilleB79
Copy link
Collaborator

@seanbudd, I know that this PR has already been merged.
But regarding controlTypes, I wonder if there could be a way to log a warning only the first time that a deprecated controlTypes constant is used but not the subsequent times. This way this would still allow to have something logged but would avoid the noise due to general usage of controlType constants.
What do you think?

@seanbudd
Copy link
Member Author

seanbudd commented Jun 14, 2022

I believe this was investigated when the constants were first deprecated in #12510.
I don't think providing these warnings are a priority but if a way to do this is found a separate issue and PR is welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants