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

Move some helper functions from main IAccessibleHandler module to satisfy Linter and decrease likelihood of circular imports #12367

Merged
merged 7 commits into from May 12, 2021

Conversation

lukaszgo1
Copy link
Contributor

Link to issue number:

None - noticed when working on #12232

Summary of the issue:

Linter complains about various imports in IAccessibleHandler not being at the top of file. In the current situation moving them to the top causes errors about imports from not fully initialized module.

Description of how this pull request fixes the issue:

At first I thought this can be solved by not importing NVDAObjects.IAccessible in the api module which is unused there anyway. Unfortunately removing this import just delayed the error but NVDA still was unable to start. Therefore I've moved parts of IAccessibleHandler which are imported by various other files in this package to two new files:

Testing strategy:

With git grep inspected all usages of the moved code ensured none were missed
Enabled MSAA logging category in the advanced settings and made sure than expected info is still logged.

Known issues with pull request:

None known

Change log entries:

None needed IMHO. I don't think anyone relied on the moved typing info - all other functions which were moved are imported back into IAccessibleHandler anyway so backward compatibility is preserved.

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual tests.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.

@lukaszgo1 lukaszgo1 marked this pull request as ready for review May 5, 2021 17:44
@lukaszgo1 lukaszgo1 requested a review from a team as a code owner May 5, 2021 17:44
@lukaszgo1 lukaszgo1 requested a review from seanbudd May 5, 2021 17:44
@lukaszgo1
Copy link
Contributor Author

cc @feerrenrut @seanbudd While I'm pretty sure this PR is not going to break backward compatibility it would be nice to merge it before 2021.1 just to be on the safe side. It is very low impact yet eliminating cases in which particular order of imports is required is always good IMHO.

@seanbudd
Copy link
Member

seanbudd commented May 10, 2021

Thanks @lukaszgo1. I agree that the code should be moved, and these changes look like a good end result. When performing the other large code refactor we came up with some problems:

  • addons which import things during the start of NVDA may trigger circular dependency issues. These changes might introduce a new one that will be hard to test for. It would be good to test this with some addons, just to catch any obvious issues.
  • it would be good to have git blame track changes across the moved functions/types. Compare the output of git blame -M -C source\IAccessibleHandler\utils.py and git blame -M -C -w source\IAccessibleHandler\utils.py
    • if the whitespace changes are removed and added to a linting commit, we can set git blame to ignore this revision
    • we plan to make a ignore-revs file for this to fix Linting was disabled on speech.py #12377

With the other refactor, we removed linting so that a large linting change with an ignore-revs file wouldn't hold up the release. This might be a strategy to employ here, but at the very least the linting/whitespace changes should be done in a single commit if possible. Since this isn't an API breaking change, and we have very little holding back the release now, I think it's unlikely we will be able to squeeze this in.

@feerrenrut feerrenrut added this to the 2021.1 milestone May 11, 2021
@lukaszgo1 lukaszgo1 force-pushed the IAccessibleHandlerReorderImports branch from ca60a66 to dd54ef6 Compare May 11, 2021 10:51
@lukaszgo1
Copy link
Contributor Author

* addons which import things during the start of NVDA may trigger circular dependency issues. These changes might introduce a new one that will be hard to test for. It would be good to test this with some addons, just to catch any obvious issues.

I've tested with my daily NVDA config (around 20 add-ons) and while this is not an exhaustive list by any means I haven't seen any errors aside from #12399 which is not caused by this PR.

* it would be good to have git blame track changes across the moved functions/types. Compare the output of `git blame -M -C source\IAccessibleHandler\utils.py` and `git blame -M -C -w  source\IAccessibleHandler\utils.py`
  
  * if the whitespace changes are removed and added to a linting commit, we can set git blame to ignore this revision

I'm not really sure why git blame fails to track these changes without ignoring white spaces since I've copied these functions verbatim (they were linted already). The only changes, aside from moving them, was to just add missing imports and copyright header to the new files. I agree that being able to track these moves is important so if you have a better idea for restructuring these commits so that git blame is able to detect these moves please do tell.

@seanbudd seanbudd force-pushed the IAccessibleHandlerReorderImports branch from dd54ef6 to 28d2a60 Compare May 12, 2021 05:04
@seanbudd
Copy link
Member

I've tested with my daily NVDA config (around 20 add-ons) and while this is not an exhaustive list by any means I haven't seen any errors aside from #12399 which is not caused by this PR.

Okay great, we should really add system tests for this as #12399 should not have been introduced.

I'm not really sure why git blame fails to track these changes without ignoring white spaces since I've copied these functions verbatim (they were linted already). The only changes, aside from moving them, was to just add missing imports and copyright header to the new files. I agree that being able to track these moves is important so if you have a better idea for restructuring these commits so that git blame is able to detect these moves please do tell.

I think your IDE or git is changing the line endings of old code with LF to CRLF. You can fix this by:

  • temporarily setting git config core.autocrlf false
  • doing a rebase on master and copy/pasting the functions again. I do this by pausing at a commit to amend and do the following
    • git checkout master fileNameOfSource (eg __init__.py)
    • git rm fileNameOfDest (eg types.py)
    • git mv fileNameOfSource fileNameOfDest
    • and then remove and unstage all the unwanted changes
    • then git commit --amend
  • ensure your IDE isn't changing line endings - I do the above to avoid but there is likely a simpler way.
  • at each step of the rebase comparing git blame -M -C source\IAccessibleHandler\utils.py and git blame -M -C -w source\IAccessibleHandler\utils.py
  • changing your git settings back eg git config core.autocrlf true

I've done a rebase doing this if @feerrenrut or yourself could review?

@seanbudd seanbudd self-assigned this May 12, 2021
@feerrenrut feerrenrut self-requested a review May 12, 2021 05:35
Copy link
Contributor Author

@lukaszgo1 lukaszgo1 left a comment

Choose a reason for hiding this comment

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

LGTM. Cannot approve since PR authors are unable to approve their own work.

@lukaszgo1
Copy link
Contributor Author

I think your IDE or git is changing the line endings of old code with LF to CRLF.

Thanks - I'll investigate at which stage this fails on my machine to hopefully be able to avoid this issue next time. Assuming that this is changed by git and not by my editor can we enforce line endings through git config for all contributors?

@seanbudd
Copy link
Member

I think your IDE or git is changing the line endings of old code with LF to CRLF.

Thanks - I'll investigate at which stage this fails on my machine to hopefully be able to avoid this issue next time. Assuming that this is changed by git and not by my editor can we enforce line endings through git config for all contributors?

This would be good to have, we would also like to normalize all files to CRLF and add the commit to an ignore-revs file at some point

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

LGTM, but will let @feerrenrut have a chance to have a look at it

@feerrenrut
Copy link
Contributor

Yep this looks ok. But I think there should be a description of these changes in the "for developers section", it may be unlikely, but we can't say it's impossible there are no addons relying on these things.

@seanbudd seanbudd force-pushed the IAccessibleHandlerReorderImports branch from b0bacac to 447172d Compare May 12, 2021 08:28
@seanbudd seanbudd merged commit 60bf94e into nvaccess:master May 12, 2021
@lukaszgo1 lukaszgo1 deleted the IAccessibleHandlerReorderImports branch May 12, 2021 08:45
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.

None yet

3 participants