Skip to content
This repository has been archived by the owner on Dec 22, 2021. It is now read-only.

Initial demo version #7

Merged
merged 41 commits into from
Jun 26, 2012
Merged

Initial demo version #7

merged 41 commits into from
Jun 26, 2012

Conversation

ryanpitts
Copy link
Contributor

No description provided.

@davidascher
Copy link
Contributor

There seems to be a lot in this pull request which isn't specific to the site, and generic playdoh stuff. Is it possible to split it into two?

@ryanpitts
Copy link
Contributor Author

Shoot, I apologize if I'm not setting this up correctly. I've been operating with firefox-flicks as the example of how to set up a Playdoh app (https://github.com/mozilla/firefox-flicks), and thought all this was supposed to be there.

Or do you mean split into two pull requests?


Ryan Pitts
@ryanpitts
http://www.ryanpitts.com

On Tuesday, June 12, 2012 at 11:05 AM, davidascher wrote:

There seems to be a lot in this pull request which isn't specific to the site, and generic playdoh stuff. Is it possible to split it into two?


Reply to this email directly or view it on GitHub:
#7 (comment)

@davidascher
Copy link
Contributor

Yeah I'm thinking that it'd be good to see the app specific stuff in its
own pull request.

@ryanpitts
Copy link
Contributor Author

OK, closing this one then.

@ryanpitts ryanpitts closed this Jun 12, 2012
@ryanpitts
Copy link
Contributor Author

It doesn't look like I can split the pull request in two, because everything was pushed to my fork in the same commit. I think the only way to make that happen would be to destroy my fork and create a new one, and then commit in two parts.

Unless I'm missing an alternate method. Any thoughts?

@ryanpitts ryanpitts reopened this Jun 13, 2012
@jbuck
Copy link
Member

jbuck commented Jun 14, 2012

You can use git rebase --interactive to split your commits into smaller commits; if you'd like some help with this, feel free to ask

@ryanpitts
Copy link
Contributor Author

Ah! That's really good to know. Thank you.

On Jun 14, 2012, at 2:27 PM, Jon Buckleyreply@reply.github.com wrote:

You can use git rebase --interactive to split your commits into smaller commits; if you'd like some help with this, feel free to ask


Reply to this email directly or view it on GitHub:
#7 (comment)

@cmcavoy
Copy link
Contributor

cmcavoy commented Jun 15, 2012

The diff is big, so it's hard to do in-line comments. Instead, I have a series of bullets...

  • a few requirements were missing, I added them in a fork and am going to submit them to you as a pull request against your fork. Adding requirements to a playdoh app is a 2 step process, it's kind of a pain. Check out their docs.
  • (I think this has been covered...but...) commits are cheap, make more of them...it helps to break things up a bit and see what you're thinking.
  • Don't check in sqlite databases to revision control...instead use fixtures...
  • the organization of apps is a little odd, you have them organized by broad types, which isn't terrible...just probably more trouble than it's worth. This is more of a talk it through in person or on the phone kind of note...it's subjective, so take it with a grain of salt. I like organizing apps as python modules (which they are), so thinking of it as a thing I'd reuse in another project...anyway, I'd like to hear your logic one of these days.
  • moz security reviews will want you to put the admin behind VPN...fyi.
  • no tests...shaaaaaaameful ;)
  • given the relatively static nature of the app, I'd suggest adding some caching.

All in all, it's a good app, pretty straight forward. I don't see anything to be worried about.

@ryanpitts
Copy link
Contributor Author

Awesome, great comments. I'll check that pull request.

Caching and tests are definitely on my task list, I just needed to get something workable up and running quickly for a demo deadline. That's what I was thinking re: the sqlite db as well, but now that you mention it, I don't know why I didn't just include fixtures.

I split the apps into those categories because initially I was thinking there might be a lot more to each one of them. Feels like they could pretty easily be pulled back together into one master app, though, especially with how tightly intertwined those content types are. Did you have a different though on how you might organize those? Would love to hear it.

@ryanpitts
Copy link
Contributor Author

Some commits from over the weekend, when I had a chance to work directly with Erin. Nothing that changes structure of the app or adds any dependencies. Just a few field tweaks, and the addition of fixtures that include real demo data. Once this points at a stable db, loaddata test_data should get things up and going immediately.

@rossbruniges
Copy link
Contributor

Ryan, have just seen that you've committed your local.py to the repo. You're going to want to delete this from the pull request and add it to the .gitignore

If there are default values you want people to have when they set it up locally add those to the local.py-dist.

@ryanpitts
Copy link
Contributor Author

You know, I was wondering about that. The .gitignore created by the Playdoh install includes a line for local_settings.py, so maybe that's an artifact from when Playdoh was arranged a bit differently?

This makes total sense, will push a fix right now.

@rossbruniges
Copy link
Contributor

Yeah that's exactly right - settings_local.py was from way back when you
had an apps directory at the root of the project.

Then since found out that was a bad idea so moved it into it's own
directory (in this case source)

Feel free to send playdoh a pull request - one for the .gitignore and one
for their docs :)

On Thu, Jun 21, 2012 at 5:52 PM, Ryan Pitts <
reply@reply.github.com

wrote:

You know, I was wondering about that. The .gitignore created by the
Playdoh install includes a line for local_settings.py, so maybe that's an
artifact from when Playdoh was arranged a bit differently?

This makes total sense, will push a fix right now.


Reply to this email directly or view it on GitHub:
#7 (comment)

@ryanpitts
Copy link
Contributor Author

Interesting, the Playdoh .gitignore looks fine.

https://github.com/mozilla/playdoh/blob/master/.gitignore

So maybe this is a problem when you use funfactory to create the project?


Ryan Pitts
@ryanpitts
http://www.ryanpitts.com

On Thursday, June 21, 2012 at 10:00 AM, Ross Bruniges wrote:

Yeah that's exactly right - settings_local.py was from way back when you
had an apps directory at the root of the project.

Then since found out that was a bad idea so moved it into it's own
directory (in this case source)

Feel free to send playdoh a pull request - one for the .gitignore and one
for their docs :)

On Thu, Jun 21, 2012 at 5:52 PM, Ryan Pitts <
reply@reply.github.com (mailto:reply@reply.github.com)

wrote:

You know, I was wondering about that. The .gitignore created by the
Playdoh install includes a line for local_settings.py, so maybe that's an
artifact from when Playdoh was arranged a bit differently?

This makes total sense, will push a fix right now.


Reply to this email directly or view it on GitHub:
#7 (comment)


Reply to this email directly or view it on GitHub:
#7 (comment)

@rossbruniges
Copy link
Contributor

My guess is that the playdoh one gets trumped by the one in root, or that
it applies only to the playdoh lib directory.

On Thu, Jun 21, 2012 at 6:13 PM, Ryan Pitts <
reply@reply.github.com

wrote:

Interesting, the Playdoh .gitignore looks fine.

https://github.com/mozilla/playdoh/blob/master/.gitignore

So maybe this is a problem when you use funfactory to create the project?


Ryan Pitts
@ryanpitts
http://www.ryanpitts.com

On Thursday, June 21, 2012 at 10:00 AM, Ross Bruniges wrote:

Yeah that's exactly right - settings_local.py was from way back when you
had an apps directory at the root of the project.

Then since found out that was a bad idea so moved it into it's own
directory (in this case source)

Feel free to send playdoh a pull request - one for the .gitignore and one
for their docs :)

On Thu, Jun 21, 2012 at 5:52 PM, Ryan Pitts <
reply@reply.github.com (mailto:reply@reply.github.com)

wrote:

You know, I was wondering about that. The .gitignore created by the
Playdoh install includes a line for local_settings.py, so maybe that's
an
artifact from when Playdoh was arranged a bit differently?

This makes total sense, will push a fix right now.


Reply to this email directly or view it on GitHub:
#7 (comment)


Reply to this email directly or view it on GitHub:
#7 (comment)


Reply to this email directly or view it on GitHub:
#7 (comment)

rossbruniges added a commit that referenced this pull request Jun 26, 2012
Initial demo version
Been given the thumbs up by Chris McAvoy and Ross Bruniges
@rossbruniges rossbruniges merged commit 69f8aae into mozilla:master Jun 26, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants