Skip to content
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

Ania and Suze #1

Open
wants to merge 70 commits into
base: master
Choose a base branch
from
Open

Ania and Suze #1

wants to merge 70 commits into from

Conversation

SuzHarrison
Copy link

We did our best to get through the main requirements, but did not get to testing every end point and the requirements added Monday. Some testing passing for models and we were able to set up a testing db.

SuzHarrison and others added 30 commits June 14, 2016 14:55
…. Seed data now loads, and includes logic to take care of asynchronous seeding for customers and movies.
 Please enter the commit message for #your changes. Lines starting
var CustomersController = {

getCustomers: function(req, res, next) {
// giving a callback function to handle error or render view
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to remove any unnecessary comments before submitting your final PR. They are super helpful during development, though, huh? :)

@knaydee
Copy link

knaydee commented Jul 5, 2016

Hey @SuzHarrison and @aniagonzalez, great job! 🎉⭐️👍 I know close to nothing about Express so it was difficult to do a CR in that respect. Looks like you got a pretty good handle on it though. Love your commit messages (very detailed) and how often you committed. Two take aways I would suggest: remove any unnecessary comments from code and be as descriptive as you can in naming variables, methods, etc.

@@ -0,0 +1,10 @@
SELECT

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this query is the same as the previous one. I wonder if they're both necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants