-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
fixed mypy error and change source var #15853
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
fixed mypy error and change source var #15853
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 82a4897
Previous suggestions✅ Suggestions up to commit 1ee594a
|
ran these two commands - are these the correct tests to run for this file ? @navin772 , can you kindly please guide. thanks. |
This is the test command - |
ran this - pytest bazel-selenium/py/test/selenium/webdriver/common/w3c_interaction_tests.py py/test/selenium/webdriver/common/w3c_interaction_tests.py:251: AssertionError @navin772 -- i don't think it is related. is it? |
please run the formatter on your branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having source
and self.input_source
refer to the same thing is confusing. I think leaving the old name is better.
Is there a reason for renaming _key_action
to keyaction
? Removing the leading underscore means it is public, where it was private previously. Also, key_action
better follows our snake case naming convention.
@cgoldberg if i don't change the source var, the w3c test throws error -using another variable to call on actual object then str. --i changed the key action method, thank you for helping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i don't change the source var, the w3c test throws error
How did it work correctly before your change?
Could you remove the 3 comments you added in your last change? They aren't needed.
@cgoldberg w3c tests pass. with separating these vars, w3c tests passes and mypy errors get fixed. i am renaming the variables- looked at the pointer, wheel and key input files. mypy errors arent there now. please let me know if there is anything else? i am trying to understand. thanks Corey. |
OK, I understand now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. thanks!
Thank you @cgoldberg . |
User description
🔗 Related Issues
💥 What does this PR do?
fixed mypy error and change source var
🔧 Implementation Notes
💡 Additional Considerations
🔄 Types of changes
PR Type
Bug fix, Enhancement
Description
Renamed
source
attribute toinput_source
for clarityFixed mypy type error in
KeyActions
classUpdated superclass initialization to use correct argument
Adjusted internal method to use new attribute name
Changes walkthrough 📝
key_actions.py
Refactor KeyActions for type safety and clarity
py/selenium/webdriver/common/actions/key_actions.py
self.source
toself.input_source
throughout the class__init__
call to useKEY
instead ofsource