Skip to content

Mark globalVars for deprecation#14050

Merged
seanbudd merged 10 commits into
masterfrom
markGlobalVarsForDeprecation
Aug 30, 2022
Merged

Mark globalVars for deprecation#14050
seanbudd merged 10 commits into
masterfrom
markGlobalVarsForDeprecation

Conversation

@seanbudd

@seanbudd seanbudd commented Aug 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Link to issue number:

Supersedes #14037
See also #14049

Summary of the issue:

#14037 was closed because there was no way to preserve backwards compatibility.
This is the case for any module level variable which the NVDA API expects to support assignment (see also #14049).

globalVars contains many NVDA state variables which should not be changed by add-on authors.
As a result, globalVars should eventually be deprecated, in favour of encapsulation of these variables.
This will make retaining backwards compatibility possible in the future.

Description of user facing changes

None

Description of development approach

Added a deprecation warning and strategy to the docstring of globalVars.

Encapsulated some less used globalVars to provide an example of what the future API should look like.

runningAsSource was moved from globalVars as this is an unreleased variable. (#14015)
_allowDeprecatedAPI was moved as it is internal code.

Testing strategy:

None

Known issues with pull request:

None

Change log entries:

None

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
  • Security precautions taken.

@seanbudd seanbudd requested a review from a team as a code owner August 23, 2022 02:03
@seanbudd seanbudd requested review from feerrenrut and removed request for a team August 23, 2022 02:03
This was referenced Aug 23, 2022
Merged
Closed
feerrenrut
feerrenrut suggested changes Aug 25, 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.

Generally looks good.

Are there any removed attributes from globalVars.py, it looks like runningAsSource was removed. Can you confirm in the description the removed attributes, and why it is ok (I assume runningAsSource was added in this release? If so a link to the PR that added it would help also.)

Comment thread source/globalVars.py
Comment thread source/globalVars.py Outdated
@LeonarddeR LeonarddeR mentioned this pull request Aug 25, 2022
Merged
6 tasks
Base automatically changed from addNotesOnModuleLevelVars to master August 26, 2022 01:07
seanbudd added a commit that referenced this pull request Aug 26, 2022
@seanbudd
Add notes on limitations to retaining backwards compatibility (#14049)
5a4512e 
See also #14050

Summary of the issue:
#14037 was closed because there was no way to preserve backwards compatibility.
This is the case for any module level variable which the NVDA API expects to support assignment.

Description of user facing changes
None

Description of development approach
Documentation has been added:

to warn developers about writing code which allows for future changes without breaking backwards compatibility.
keep track of cases which we cannot safely retain backwards compatibility, so that we can clearly define when API breaking changes are necessary
@seanbudd seanbudd marked this pull request as draft August 26, 2022 01:07
@nvaccessAuto nvaccessAuto added this to the 2022.4 milestone Aug 26, 2022
@seanbudd

seanbudd commented Aug 26, 2022

Copy link
Copy Markdown
Member Author

Are there any removed attributes from globalVars.py, it looks like runningAsSource was removed. Can you confirm in the description the removed attributes, and why it is ok (I assume runningAsSource was added in this release? If so a link to the PR that added it would help also.)

runningAsSource was moved from globalVars as this is an unreleased variable. (#14015)
_allowDeprecatedAPI was moved as it is internal code.

@seanbudd seanbudd requested a review from feerrenrut August 26, 2022 04:35
@seanbudd seanbudd marked this pull request as ready for review August 26, 2022 04:36
@seanbudd seanbudd self-assigned this Aug 26, 2022
@seanbudd seanbudd added the release/blocking this issue blocks the milestone release label Aug 26, 2022
@seanbudd seanbudd closed this in 8fc092c Aug 27, 2022
@seanbudd seanbudd reopened this Aug 27, 2022
@AppVeyorBot

AppVeyorBot commented Aug 27, 2022

Copy link
Copy Markdown
  • PASS: Translation comments check.
  • PASS: Unit tests.
  • FAIL: Lint check. See test results for more information.
  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 52a1c6eed4

@AppVeyorBot

AppVeyorBot commented Aug 29, 2022

Copy link
Copy Markdown
  • PASS: Translation comments check.
  • PASS: Unit tests.
  • FAIL: Lint check. See test results for more information.
  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit c197318d3b

@AppVeyorBot

AppVeyorBot commented Aug 30, 2022

Copy link
Copy Markdown

See test results for failed build of commit 4c6564a0ea

@AppVeyorBot

AppVeyorBot commented Aug 30, 2022

Copy link
Copy Markdown

See test results for failed build of commit 4206bf2207

@seanbudd seanbudd merged commit 4721336 into master Aug 30, 2022
@seanbudd seanbudd deleted the markGlobalVarsForDeprecation branch August 30, 2022 04:15
seanbudd added a commit that referenced this pull request Nov 3, 2022
@seanbudd
restore API import
901699a 
#14050 introduced a usage of api to core.main to beta\n#14301 moved the api import from core.main as it was otherwise unused on rc
@seanbudd seanbudd mentioned this pull request Nov 3, 2022
Merged
6 tasks
seanbudd added a commit that referenced this pull request Nov 3, 2022
@seanbudd
restore API import to core.main on beta (#14330)
48caf54 
Summary of the issue:
#14301 moved an api import from core.main as it was otherwise unused
#14050 introduced a usage of api to core.main to beta
This means that api was used without being imported on beta and master.

Description of user facing changes
None

Description of development approach
Fix import error
@CyrilleB79

CyrilleB79 commented Nov 30, 2022

Copy link
Copy Markdown
Contributor

@seanbudd, @feerrenrut , why isn't there any change log item regarding these deprecations? If add-on authors are meant not to use these variables, it's a pity that this deprecation has not been advertised.

Unfortunately, the translation period is over for 2022.4.

There are various options:

  1. add this information in the log and proceed to a new translation freeze.
  2. add this information in the log in English only; the information will then translated a posteriori for 2023.1 release. And in parallel, widely advertise of this untranslated modification, e.g. on API mailing-list, on NVDA devel and on translators mailing list.

Fortunately, it's only a matter of deprecation, not of removal.

What do you think?

@zstanecic

zstanecic commented Nov 30, 2022 via email •
edited by feerrenrut
Loading

Copy link
Copy Markdown
Contributor

@seanbudd

seanbudd commented Dec 1, 2022

Copy link
Copy Markdown
Member Author

I don't think it is too late for 2022.4. We can reannounce the translation freeze.

@seanbudd

seanbudd commented Dec 1, 2022

Copy link
Copy Markdown
Member Author

Announcing this was avoided, as there is currently no clear alternative for many of these variables in NVDA core.
The change here was intended as internal documentation for NVDA core, to outline a strategy for future deprecation of the module.
I don't believe add-on authors are expected to avoid relying on any of these symbols.

@CyrilleB79

CyrilleB79 commented Dec 4, 2022

Copy link
Copy Markdown
Contributor

Announcing this was avoided, as there is currently no clear alternative for many of these variables in NVDA core. The change here was intended as internal documentation for NVDA core, to outline a strategy for future deprecation of the module. I don't believe add-on authors are expected to avoid relying on any of these symbols.

OK thanks for this clarification. At the beginning, I found a bit confusing the fact that there is a deprecation string below each variable. But it's true that the strategy described on top of the module is clear and your last comment is a double confirmation.

It will be enough to indicate the deprecation in the change log when a deprecation warning at module level is implemented.

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

@seanbudd seanbudd

Labels

release/blocking this issue blocks the milestone release

Projects

None yet

Milestone

2022.4

Development

Successfully merging this pull request may close these issues.

6 participants

@seanbudd @AppVeyorBot @CyrilleB79 @zstanecic @feerrenrut @nvaccessAuto