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

Remove unnecessary format subclass methods #76

Merged
merged 3 commits into from
Aug 29, 2019
Merged

Conversation

rjgildea
Copy link

@rjgildea rjgildea commented Aug 28, 2019

Remove unnecessary __init__ and _start methods from Format classes:

  • There is no need for every subclass to redefine the __init__ method and
    call its parent's __init__ method. As well as a lot of unnecessary code
    duplication, this also led to the subclass self.understand() method
    being called once for every class in the format class hierarchy.
  • There is no need for every subclass to redefine the _start() method and
    call its parent's _start() method.
  • Use super() where applicable.

There is no need for every subclass to redefine the __init__ method and
call its parent's __init__ method. As well as a lot of unnecessary code
duplication, this also led to the subclass self.understand() method
being called once for every class in the format class hierarchy.
Use super() where applicable.
There is no need for every subclass to redefine the _start() method and
call its parent's _start() method.
Use super() where applicable.
@graeme-winter
Copy link
Collaborator

Reviewing...

@graeme-winter
Copy link
Collaborator

0-th order - tests:

Master:

Results (47.73s):
     504 passed
       1 xfailed
      15 skipped

Branch:

Results (117.56s):
     503 passed
       1 xfailed
      15 skipped

Interesting that there were fewer tests 🤔 possibly one added since you made the branch. Time difference due to nproc= for first not second 🙄

Copy link
Collaborator

@graeme-winter graeme-winter left a comment

Choose a reason for hiding this comment

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

Looked through the changes (after the first few, scanned diffs) and they look fine.

Tests pass effectively the same before and afterwards. No evident regressions.

=> 👍

@graeme-winter
Copy link
Collaborator

I should also say: this is a good thing and will help newer developers as there is (slightly less) cargo culted nonsense in there.

@rjgildea
Copy link
Author

rjgildea commented Aug 29, 2019

Interesting that there were fewer tests 🤔 possibly one added since you made the branch.

@Anthchirp split one test into two here which probably accounts for the differing number of test compared to master: dials@8c169ff

@Anthchirp
Copy link
Member

I now removed one on master, so count should be identical again 😏

@rjgildea rjgildea merged commit 1214833 into master Aug 29, 2019
@Anthchirp Anthchirp deleted the cleanup-format-classes branch August 29, 2019 20:37
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.

3 participants