Skip to content

Extend the life of deprecated aliases#13791

Merged
seanbudd merged 4 commits into
betafrom
extendDeprecatedAliases
Jun 14, 2022
Merged

Extend the life of deprecated aliases#13791
seanbudd merged 4 commits into
betafrom
extendDeprecatedAliases

Conversation

@seanbudd

@seanbudd seanbudd commented Jun 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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
@seanbudd seanbudd mentioned this pull request Jun 14, 2022
Closed
5 tasks
@AppVeyorBot

AppVeyorBot commented Jun 14, 2022

Copy link
Copy Markdown

See test results for failed build of commit 70f38a5108

feerrenrut
feerrenrut suggested changes Jun 14, 2022
View reviewed changes
Comment thread source/appModuleHandler.py Outdated
Comment thread source/globalVars.py Outdated
Comment thread source/config/__init__.py Outdated
Comment thread source/appModuleHandler.py Outdated
@seanbudd seanbudd requested a review from feerrenrut June 14, 2022 02:59
feerrenrut
feerrenrut approved these changes Jun 14, 2022
View reviewed changes

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

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

seanbudd commented Jun 14, 2022

Copy link
Copy Markdown
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.

Sign in to view
@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

CyrilleB79 commented Jun 14, 2022

Copy link
Copy Markdown
Contributor

@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

seanbudd commented Jun 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

Reviewers

1 more reviewer

@feerrenrut feerrenrut feerrenrut approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2022.3

Development

Successfully merging this pull request may close these issues.

5 participants

@seanbudd @AppVeyorBot @CyrilleB79 @feerrenrut @nvaccessAuto