replace controlTypes.STATE_* with controlTypes.State.* by seanbudd · Pull Request #12712 · nvaccess/nvda

seanbudd · 2021-08-05T02:48:35Z

Link to issue number:

Part of #12549

Summary of the issue:

controlTypes.STATE_* are deprecated, to be replaced by controlTypes.State.*

Description of how this pull request fixes the issue:

replaces all instances of controlTypes.STATE_ with controlTypes.State. across .py files
search and manually replace final STATE_ constant usages
- Searched using the regex [^\.]\bSTATE_, which finds STATE_ instances
  where:
  - it is not of the form <Foo>.STATE_ (as oleacc.STATE_* exists)
  - STATE_ is the prefix of the symbol (as IA2_STATE_* 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 and negativeDisplayString 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 [^\.]\bSTATE_, which finds STATE_ instances where * it is not of the form <Foo>.STATE_ (as oleacc.STATE_* exists) * STATE_ is the prefix of the symbol (as IA2_STATE_* exists) Replaced the remaining instances manually where required. Fixes unit tests to cover new usages and legacy usages.

AppVeyorBot · 2021-08-05T03:04:06Z

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/garuexpj0h2jv0n6/artifacts/output/nvda_snapshot_pr12712-23534,213fb3df.exe

See test results for failed build of commit 213fb3dfb0

feerrenrut

Overall looks good.

I've been thinking "how do we prevent new code from using the old STATE_* constants?" Perhaps we could extend the lint check (which already creates a diff), then regex on that diff looking for STATE_* constants.

seanbudd · 2021-08-09T04:56:03Z

I've been thinking "how do we prevent new code from using the old STATE_* constants?" Perhaps we could extend the lint check (which already creates a diff), then regex on that diff looking for STATE_* constants.

How would we determine STATE_* constant usage? Right now it is manual confirmation (with a regex guide).

Once this is merged in, we could replace the mapping STATE_* = State.* with the following idea from @LeonarddeR #12649 (comment), and it would be less noisy. This is a suggested usage of PEP-562.

def __getattr__(name: str) -> Any:
	if name.startswith("STATE_"):
		log.error("Deprecated constant used")
		return getattr(State, name.split("_", 1)[-1])

AppVeyorBot · 2021-08-09T04:58:45Z

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/bwlqtwcntindbjh8/artifacts/output/nvda_snapshot_pr12712-23557,5408e6d2.exe

See test results for failed build of commit 5408e6d2b8

lukaszgo1 · 2021-08-09T10:31:16Z

I've been thinking "how do we prevent new code from using the old STATE_* constants?" Perhaps we could extend the lint check (which already creates a diff), then regex on that diff looking for STATE_* constants.

How would we determine STATE_* constant usage? Right now it is manual confirmation (with a regex guide).

Cannot we just search for every STATE_* in the diff? I agree that is not very efficient but much better than the alternative below.

Once this is merged in, we could replace the mapping STATE_* = State.* with the following idea from @LeonarddeR #12649 (comment), and it would be less noisy.

Less noisy for core but horribly annoying for add-ons making Alphas pretty unusable - imagine getting error sound every once in a while just because many add-ons are not going to use new enum's before 2022.1.

LeonarddeR · 2021-08-09T11:21:43Z

Once this is merged in, we could replace the mapping STATE_* = State.* with the following idea from @LeonarddeR #12649 (comment), and it would be less noisy.

Less noisy for core but horribly annoying for add-ons making Alphas pretty unusable - imagine getting error sound every once in a while just because many add-ons are not going to use new enum's before 2022.1.

Not noisy at all if we lower the error level a bit.

lukaszgo1 · 2021-08-09T11:53:23Z

Not noisy at all if we lower the error level a bit.

That is an option but this means that usage of the oldd constants would be harder to check for new PR's. Essentially looking for them in the diff seems to be a better option than inspecting the log as the latter solution cannot be automated.

seanbudd · 2021-08-10T00:14:26Z

Cannot we just search for every STATE_* in the diff? I agree that is not very efficient but much better than the alternative below.

There could be false positives here, eg "oleacc.STATE_*" and "IA2_STATE_*".

Not noisy at all if we lower the error level a bit.

Or if we only perform an error beep on the first runtime usage of a particular constant or group of constants.

feerrenrut · 2021-08-10T02:41:39Z

How would we determine STATE_* constant usage? Right now it is manual confirmation (with a regex guide).

By running a regex against the PR diff (Only lines changed, the same as the linter does).

There could be false positives here, eg "oleacc.STATE_" and "IA2_STATE_".

Tighten up the regex
Have a way to exclude a line / or mark as a false positive.

seanbudd · 2021-08-10T02:44:02Z

@feerrenrut - this will require a force merge as the lint check will not pass

seanbudd · 2021-08-10T02:47:57Z

Have a way to exclude a line / or mark as a false positive.

I think we can do both, and something like marking it with a comment # ignore deprecation warning.

Would adding the warnings/errors be a good idea too? I think it would be better to move to the PEP standard, and update deprecations.md.

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 August 5, 2021 12:26

replace controlTypes.STATE_ with controlTypes.State. across .py files

c6f8cff

seanbudd requested a review from feerrenrut August 5, 2021 02:48

seanbudd requested a review from a team as a code owner August 5, 2021 02:48

seanbudd mentioned this pull request Aug 5, 2021

Remove internal usage of deprecated controlTypes constants #12549

Closed

5 tasks

feerrenrut reviewed Aug 9, 2021

View reviewed changes

Comment thread source/appModules/powerpnt.py Outdated

seanbudd added 3 commits August 9, 2021 14:36

Merge branch 'master' into STATE-to-State

82e7297

fix line endings for powerpnt

39def2d

fix line endings for powerpnt

067a402

feerrenrut approved these changes Aug 10, 2021

View reviewed changes

feerrenrut merged commit 17e5286 into master Aug 10, 2021

feerrenrut deleted the STATE-to-State branch August 10, 2021 03:04

nvaccessAuto added this to the 2021.3 milestone Aug 10, 2021

LeonarddeR mentioned this pull request Jul 19, 2024

Add prs to blame ignore refs for historical refactors #16886

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

replace controlTypes.STATE_* with controlTypes.State.*#12712
feerrenrut merged 5 commits into
masterfrom
STATE-to-State

seanbudd commented Aug 5, 2021

Uh oh!

AppVeyorBot commented Aug 5, 2021

Uh oh!

feerrenrut left a comment

Uh oh!

Uh oh!

seanbudd commented Aug 9, 2021 •

edited

Loading

Uh oh!

AppVeyorBot commented Aug 9, 2021

Uh oh!

lukaszgo1 commented Aug 9, 2021

Uh oh!

LeonarddeR commented Aug 9, 2021

Uh oh!

lukaszgo1 commented Aug 9, 2021

Uh oh!

seanbudd commented Aug 10, 2021 •

edited

Loading

Uh oh!

feerrenrut commented Aug 10, 2021

Uh oh!

seanbudd commented Aug 10, 2021

Uh oh!

seanbudd commented Aug 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

seanbudd commented Aug 5, 2021

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 Aug 5, 2021

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

AppVeyorBot commented Aug 9, 2021

Uh oh!

lukaszgo1 commented Aug 9, 2021

Uh oh!

LeonarddeR commented Aug 9, 2021

Uh oh!

lukaszgo1 commented Aug 9, 2021

Uh oh!

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

Uh oh!

Uh oh!

feerrenrut commented Aug 10, 2021

Uh oh!

seanbudd commented Aug 10, 2021

Uh oh!

seanbudd commented Aug 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

seanbudd commented Aug 9, 2021 •

edited

Loading

seanbudd commented Aug 10, 2021 •

edited

Loading