This site is an archive; learn more about 8 years of OpenHatch.

[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