[OH-Dev] Replace controllers with helpers
Asheesh Laroia
asheesh at asheesh.org
Mon Apr 1 21:19:00 UTC 2013
On Sat, 30 Mar 2013, Rajesh Pradhan wrote:
> Sorry wrong url in my previous mail for the pull request.
>
> https://github.com/openhatch/oh-mainline/pull/102
#102 looks awesome. I'm testing it more thoroughly now, but on a
preliminary review it:
* Passes the tests (which is a HUGE hurdle for a massive refactor like
this)
* Moves the code to the new filenames, rather than just copying it
There are only a few things I would suggest hypothetically changing, if
you were doing it again:
* Use 'git rebase -i' to 'squash' some of the commits into each other. In
particular, the commit where you remove controllers.py from the missions
app (commit log message: "replaced controllers with view_helpers in
missions app") and the next commit (message: "view_helpers.py added in
missions app") should be the same.
If they are the same, then git will detect the rename as a rename, rather
than as a deletion then addition. However, this has little
practical consequence, so it's not something worth resubmitting
over.
Also:
* Add a note to the LAYOUT file indicating what view_helpers.py is for. (I
can do that myself, once I merge this.)
I'm running it through the MySQL tests, which you yourself can do with:
USE_MYSQL=1 python manage.py test
and comparing that to origin/master now. I expect it to have the same
amount passing as origin/master (which, sadly, is not 100%), and then I
can't think of any objections... so I suppose I will deploy it!
(Also, I presumed request 101 was to be ignored now because I saw it is
"closed" in Github.)
Thank you for this heavy lifting! Once this lands, you've made the
OpenHatch codebase noticeably simpler to understand.
If refactoring like this is something you enjoy, then I can happily
suggest another refactoring task! (-: Either way, I hope you keep
contributing!
-- Asheesh.
More information about the Devel
mailing list