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

Add missing calls to super class "__init__"s #1693

Merged
merged 7 commits into from
Jul 7, 2021

Conversation

rahulporuri
Copy link
Contributor

This PR adds missing calls to super().__init__() in a few classes which overrided __init__ methods of their base classes.

Checklist

  • Add a news fragment if this PR is news-worthy for end users. (see docs/releases/README.rst)

Poruri Sai Rahul added 5 commits June 21, 2021 12:01
@rahulporuri
Copy link
Contributor Author

One of the CI jobs failed with the following weird/cryptic message while running the integration tests. Restarting the jobs to see if this is intermittent.

Command '['edm', 'run', '-e', 'traitsui-test-3.6-pyside2', '--', 'python', '-W', 'default', '-m', 'unittest', 'discover', '-v', '/home/runner/work/traitsui/traitsui/integrationtests']' returned non-zero exit status 252.

@aaronayres35
Copy link
Contributor

One of the CI jobs failed with the following weird/cryptic message while running the integration tests. Restarting the jobs to see if this is intermittent.

Command '['edm', 'run', '-e', 'traitsui-test-3.6-pyside2', '--', 'python', '-W', 'default', '-m', 'unittest', 'discover', '-v', '/home/runner/work/traitsui/traitsui/integrationtests']' returned non-zero exit status 252.

This just happened on #1674 as well... 🤔

Copy link
Contributor

@aaronayres35 aaronayres35 left a comment

Choose a reason for hiding this comment

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

LGTM

My only question is how did you decide where to place the call to super within the body of the method?

Unsure about the CI failures though. Re-triggering CI seemed to fix the issue on #1674, but it doesn't look like that is the case here.

@rahulporuri
Copy link
Contributor Author

My only question is how did you decide where to place the call to super within the body of the method?

I was trying to do it as close to the top of the method as possible - while also setting any arguments on the class that were passed through.

Poruri Sai Rahul added 2 commits July 7, 2021 19:53
Previously, we were setting traits/attributes before calling __init__ on
the objects, which setup the traits machinery. Instead, this commit
passes the arguments to __init__, which feels like the right thing to do

	modified:   traitsui/editors/array_editor.py
	modified:   traitsui/editors/tuple_editor.py
	modified:   traitsui/qt4/ui_panel.py
@rahulporuri
Copy link
Contributor Author

@aaronayres35 given your comment, i went with a slightly different approach to this PR - i passed the incoming arguments to the super __init__ call - instead of setting it on the object in the __init__ method. This new approach feels like the right thing to do - and is something i've seem elsewhere as well.

@rahulporuri
Copy link
Contributor Author

rahulporuri commented Jul 7, 2021

given the changes, would you like to do another review of this PR @aaronayres35 ?

Copy link
Contributor

@aaronayres35 aaronayres35 left a comment

Choose a reason for hiding this comment

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

Still LGTM, thank you!

@rahulporuri rahulporuri merged commit 762f66b into master Jul 7, 2021
@rahulporuri rahulporuri deleted the fix/add-missing-super-calls branch July 7, 2021 15:39
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.

2 participants