-
Notifications
You must be signed in to change notification settings - Fork 109
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
Group classes by function or type #196
Group classes by function or type #196
Conversation
…orm similar functionality - first pass, WIP
9b834cc
to
ab70ab8
Compare
run-pylint-check.sh
Outdated
@@ -0,0 +1,12 @@ | |||
#!/bin/bash |
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.
Why do we need this file?
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.
To enable running checks done by this command, before creating a PR, its similar to what you have done in the travis-ci builds
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.
Ok but I don't think we need it in the repo. Can you please remove it.
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.
Done! Removed!
Given the size of the diff with all this rearranging, please make sure the PR contains only rearranging so that it should be trivially equivalent to the original code. Note that there's still a risk in breakage when moving module level statements (which includes class and method definitions) but hopefully our gate with catch any problems related to that. |
Yes I have taken care to see there is no reason for breakage - the IDE is helping, plus I'm building using the build script to run the changed version of mx each time. These changes are just moving the blocks closer to each other so they can be extracted. There is still a chance of breakage, and this could indicate to us something when it happens either code that needs fixing, usually less optimal couplings can cause it. |
I'll rebase mx.py to fix the conflict reported by github. |
Please don't rebase. If I cannot resolve conflicts when integrating, I'll reach out for help. Please let me know once you're ready for me to try integrate this PR. |
Please proceed, this PR is good to go! |
@dougxc any chance of this being reviewed if you have the bandwidth? |
I'm trying to integrate it now. |
For anybody ever needing to review this kind of changes, this was reviewed using this script based on an idea from @ansalond. |
Thanks for letting us know. Let's hope we don't have to do reviews on such file for long! |
Thanks! |
Good work with the script - nice work! I just applied it to the PR #197 I think the |
I have created a bash script which can be run after running your Using |
I think it is (and should remain!) rather rare to make changes that shuffle everything around. It is not about reviewing large source files (the standard tools do that just fine), it's about reviewing changes which just re-order a lot of things (regardless of the size of the files). |
On the back of #194 I have started to slowly group together classes, methods/functions, fields/variables that come across as fulfilling similar functionality.
For this sweep I picked these 3 categories:
Doing this should help identify temporal coupling, preparing the file for refactoring later on.
I re-ran the graal build via https://circleci.com/gh/neomatrix369/awesome-graal/503 which builds this particular repo and branch.
Build status of this branch: