search

atoponce / d-note

Self destructing encrypted notes
Other
130 stars 43 forks source link

Refactor code to separate Model from Controller #31

Closed fungus closed 10 years ago

fungus commented 10 years ago
fungus commented 10 years ago

With such a big change as this, I wanted to start a discussion with this pull request.

atoponce commented 10 years ago

Checking over the code now. I'll have some comments later.

atoponce commented 10 years ago

So, I've looked it over, and I'm impressed. I was hoping to tackle fetch_url(). It was U.G.L.Y. Even though I still think the logic could be improved, it's SUBSTANTIALLY better than previous.

However, I am curious why you didn't take duress_text() and verify_hashcash() out of __init__.py and into note.py? In fact, following the MVC model, I would think that send_email() could also be moved over. Basically, anything responsible for a page render would be in __init__.py, while the note operations in note.py. It makes sense if/when we support accounts, that we would put that code in account.py, etc.

Other than that, I don't see anything wrong with this pull request. Very clean, easy to follow and read.

fungus commented 10 years ago

I thought none of those functions belonged with the Note model. They are probably best described as their own model (verify_hashcash), or simply utility functions. Perhaps moving them to another separate file would be best?

atoponce commented 10 years ago

(I'm horrid at Github-flavored markdown)

Yeah. Moving them to a utils.py might make more sense. Also, I liked the improvements you made to secure_remove().

fungus commented 10 years ago

Thanks, and good idea. Commit added.

atoponce commented 10 years ago

So, before we push this, how should we proceed in making a "v1.0". Should we make a separate branch, or should we just create a zip file to download? Or, just keep it a rolling release? How do most developers do things with respect to releases?

atoponce commented 10 years ago

Thinking about this a little bit further, rather than a 'master' branch, we can create a 'dev' branch, and make that the default push. Then, for every release, we'll create a version branch. I think it should be simple enough to create a download .zip of that branch, for those that don't want to clone the repository.

fungus commented 10 years ago

Using different branches for releases is more of a Subversion method. With Git, one generally uses a tag on a specific commit that tags it as a release version. Then you can create a download from that tag, if desired.

As far as developing on master goes, it's fairly common, just make devs aware to try to make sure master is always working. Which means don't push your local tree to github until it's tested. Not too hard. Or for big changes, make a branch specific for the task/feature/etc. and push that as you feel.

Or more complicated or busy projects may need to separate these more and use a "develop" branch. Example: http://nvie.com/posts/a-successful-git-branching-model/

burntout commented 10 years ago

On Tue, Jun 10, 2014 at 08:09:41AM -0700, Lonnie Olson wrote:

Using different branches for releases is more of a Subversion method. With Git, one generally uses a tag on a specific commit that tags it as a release version. Then you can create a download from that tag, if desired.

As far as developing on master goes, it's fairly common, just make devs aware to try to make sure master is always working. Which means don't push your local tree to github until it's tested. Not too hard. Or for big changes, make a branch specific for the task/feature/etc. and push that as you feel.

Or more complicated or busy projects may need to separate these more and use a "develop" branch. Example: http://nvie.com/posts/a-successful-git-branching-model/

This seems more like other projects I've seen ( caveat I'm not particulary experienced ). Git repo's without a master branch look a bit weird to me; though I know it doesn't matter.

Regards,

Alan Dawson

atoponce commented 10 years ago

So, lightweight tags or annotated? Seems annotated is preferred?

fungus commented 10 years ago

Yeah, annotated tags are nice because they allow a nice release message to be logged as well.

atoponce commented 10 years ago

I'll "get 'er done". Once tagged, I'm cool with pushing this to master, and going from there.

atoponce commented 10 years ago

Okay. I pushed tag "v1.0". I think I did that correctly, if you want to check. :)

fungus commented 10 years ago

Looks good. I see the release available and downloads created. Nice. Looks ready to merge.