Replace internal controlTypes.ROLE_* usages with controlTypes.Role.* by seanbudd · Pull Request #12649 · nvaccess/nvda

seanbudd · 2021-07-15T05:05:08Z

Link to issue number:

Part of #12549

Summary of the issue:

controlTypes.ROLE_* are deprecated, to be replaced by controlTypes.Role.*

Description of how this pull request fixes the issue:

replaces all instances of controlTypes.ROLE_ with controlTypes.Role. across .py files
search and manually replace final ROLE_ constant usages
- Searched using the regex [^\.]\bROLE_, which finds ROLE_ instances
  where:
  - it is not of the form <Foo>.ROLE_ (as oleacc.ROLE_* exists)
  - ROLE_ is the prefix of the symbol (as IA2_ROLE_* exists)

Testing strategy:

Manual testing

Confirm the above steps will not introduce issues, and cover all cases.
Reproduce those steps and perform a diff with this branch

Existing unit tests exist for controlTypes, new unit tests are written to ensure displayString works

Existing system tests will work as a basic smoke test for this core feature

Known issues with pull request:

The lint check is likely to fail

Change log entries:

None needed

Code Review Checklist:

Pull Request description is up to date.
Unit tests.
System (end to end) tests.
Manual testing.
User Documentation.
Change log entry.
Context sensitive help for GUI changes.
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers

Searched using the regex [^\.]\bROLE_, which finds ROLE_ instances where * it is not of the form <Foo>.ROLE_ (as oleacc.ROLE_* exists) * ROLE_ is the prefix of the symbol (as IA2_ROLE_* exists) Replaced the remaining instances manually where required

AppVeyorBot · 2021-07-15T05:21:27Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/8vs8hkblq607lq6n/artifacts/output/nvda_snapshot_pr12649-23356,d1eb87db.exe

See test results for failed build of commit d1eb87db3a

LeonarddeR · 2021-07-15T06:53:17Z

I wonder whether we could emit deprecation warnings for old constant use, e.g. by handling them using a module getattr. For example:

def __getattr__(name: str) -> Any:
	if name.startswith"ROLE_"):
		# Warn about deprecation!
		return getattr(Role, name.split("_", 1)[-1]

josephsl · 2021-07-15T10:49:25Z

Hi,

I think we will come across linting issues for a while, given how controlTypes.ROLE_* and STATE_* are used in places where Flake8 complains about various issues.

Thanks.

seanbudd · 2021-07-16T00:31:11Z

I wonder whether we could emit deprecation warnings for old constant use, e.g. by handling them using a module getattr. For example:
def __getattr__(name: str) -> Any:
	if name.startswith"ROLE_"):
		# Warn about deprecation!
		return getattr(Role, name.split("_", 1)[-1]

My first thought is that this would be way too noisy in this instance. I would want to do this for less integral parts of NVDA, or if the removal of the deprecation is delayed and most addons uses the new enums.

I think we will come across linting issues for a while, given how controlTypes.ROLE_* and STATE_* are used in places where Flake8 complains about various issues.

In this case I think it is best to be ignored and overridden with force merges.

feerrenrut

Generally looks good. I've been through all the files.

A few files seem to have had their line endings modified, can you restore those?

Please don't force push, I'd like to be sure of which files have changed to make the follow up minimal.

lukaszgo1 · 2021-07-21T10:23:02Z

-	def _get_role(self) -> int:
+	def _get_role(self) -> controlTypes.Role:
 		"""The role or type of control this object represents (example: button, list, dialog).
-		@return: a ROLE_* constant from L{controlTypes}


This comment should be updated as well to explain that now the role value comes from the enum

I don't know what value that provides, the return type should make that clear hopefully

AppVeyorBot · 2021-07-22T01:40:33Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/meijmw8bpnhw2m11/artifacts/output/nvda_snapshot_pr12649-23398,6293c381.exe

See test results for failed build of commit 6293c38132

AppVeyorBot · 2021-07-22T01:58:55Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/k3vr1wo64qa45vsr/artifacts/output/nvda_snapshot_pr12649-23399,c207ea8a.exe

See test results for failed build of commit c207ea8abd

feerrenrut · 2021-07-26T08:43:49Z

+	def test_roleLabels(self):
+		"""Test to check whether every role has its own display string"""
+		for role in controlTypes.Role:
+			role.displayString
+


Can you take a look to see if there is simple way to add a test for the corollary, that displayString raises when a label is missing?

If this test already exists, add a comment here to point it out.

I don't think there is a simple way, as new members cannot be added to Role at run time. I've added a generic test for displayString.
If one adds a role and forgets to add a label, this test should fail assuming the displayString test still passes.

feerrenrut · 2021-07-26T08:50:45Z

Given the nature of this change, I think it is clearer not to try to address lint changes.

AppVeyorBot · 2021-07-26T09:46:40Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/s71y2a6sy6mhqup2/artifacts/output/nvda_snapshot_pr12649-23440,4c575f6e.exe

See test results for failed build of commit 4c575f6ecb

feerrenrut

Tests look good. Might as well fix that copyright year. But I'm happy for you to merge this.

AppVeyorBot · 2021-07-26T21:49:48Z

FAIL: System tests. See test results for more information.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/bagcb23oqr5suncn/artifacts/output/nvda_snapshot_pr12649-23444,3f943bbf.exe
PASS: Translation comments check.

See test results for failed build of commit 3f943bbf64

seanbudd · 2021-07-26T22:31:11Z

@feerrenrut can you merge this - I am unable with the failed lint check

AppVeyorBot · 2021-07-26T22:50:46Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/d4qorlvhu0nopm9w/artifacts/output/nvda_snapshot_pr12649-23445,3f943bbf.exe

See test results for failed build of commit 3f943bbf64

Related to #12649, #12712, #12836 Summary of the issue: The mentioned prs performed a large refactor with constant changes Description of user facing changes None known Description of development approach Add the mentioned PRs to .git-blame-ignore-revs to ensure a properly configured git blame will ignore these changes, revealing the history of the underlying code.

seanbudd added 2 commits July 15, 2021 14:47

replace controlTypes.ROLE_ with controlTypes.Role. across .py files

7b8500a

seanbudd requested a review from a team as a code owner July 15, 2021 05:05

seanbudd requested a review from feerrenrut July 15, 2021 05:05

seanbudd changed the title ~~Role to role~~ Replace internal controlTypes.ROLE_* usages with controlTypes.Role.* Jul 15, 2021

seanbudd mentioned this pull request Jul 15, 2021

Remove internal usage of deprecated controlTypes constants #12549

Closed

5 tasks

feerrenrut suggested changes Jul 21, 2021

View reviewed changes

lukaszgo1 suggested changes Jul 21, 2021

View reviewed changes

lukaszgo1 mentioned this pull request Jul 21, 2021

Fix some undefined names in NVDAObjects #12668

Merged

8 tasks

seanbudd added 2 commits July 22, 2021 11:24

fix line endings

4701d05

fix tests

5df1a24

fix specific line endings to restore mixed line endings

aa83a7e

seanbudd requested a review from feerrenrut July 26, 2021 01:46

feerrenrut suggested changes Jul 26, 2021

View reviewed changes

seanbudd added 2 commits July 26, 2021 18:58

Merge branch 'master' into ROLE-to-Role

6eda02b

add test for display string

d9ad0e1

seanbudd requested a review from feerrenrut July 26, 2021 09:33

seanbudd commented Jul 26, 2021

View reviewed changes

Comment thread tests/unit/test_util/test_displayString.py Outdated

feerrenrut approved these changes Jul 26, 2021

View reviewed changes

fix copyright year

c424041

feerrenrut merged commit defed0e into master Jul 27, 2021

feerrenrut deleted the ROLE-to-Role branch July 27, 2021 03:30

nvaccessAuto added this to the 2021.3 milestone Jul 27, 2021

seanbudd mentioned this pull request Aug 9, 2021

replace controlTypes.STATE_* with controlTypes.State.* #12712

Merged

8 tasks

LeonarddeR mentioned this pull request Jul 19, 2024

Add prs to blame ignore refs for historical refactors #16886

Merged

Uh oh!

Conversation

seanbudd commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Jul 15, 2021

Uh oh!

LeonarddeR commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephsl commented Jul 15, 2021

Uh oh!

seanbudd commented Jul 16, 2021

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukaszgo1 Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

seanbudd Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AppVeyorBot commented Jul 22, 2021

Uh oh!

AppVeyorBot commented Jul 22, 2021

Uh oh!

feerrenrut Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

seanbudd Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

feerrenrut commented Jul 26, 2021

Uh oh!

AppVeyorBot commented Jul 26, 2021

Uh oh!

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Jul 26, 2021

Uh oh!

seanbudd commented Jul 26, 2021

Uh oh!

AppVeyorBot commented Jul 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

seanbudd commented Jul 15, 2021 •

edited

Loading

LeonarddeR commented Jul 15, 2021 •

edited

Loading