Grading PR - #59
Open
martypdx wants to merge 6 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nice project overall. Good grasp of fundamentals of async coding for web server.
Nice technical write-up in the
READMEwith the application structure and execution flow description.Remove dead files and dead code (commented out). I dunno why most of your files had one or two blank lines at the start. Varying use of whitespace from file to file (e.g. between fields in schema definition). In future, strive for cleaner, more consistent code formatting.
Don't
'use strict';in node.js v6. Add this to.eslintrcto make it stop complaining:(Then add an
.eslintrcin public that sets it toscript- see PR)Great job on crafting API tests that matched actual workflows and use cases. Make sure tests are strong and test more than presence/absence of response.
eslint issues are meant to help you fix and clean up your code. If you just disable every line that has a problem, that defeats the purpose.
Problems with REST API's. Url's describe resources (nouns). Refinements are query parameters.
Would have been much cleaner and easier if you had:
createdonMany of the more complex data gets were hacky. Fine to get it working, but then improve, you had a lot of extra code that wasn't needed, often using the wrong tool for job.