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

Grading PR #41

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Grading PR #41

wants to merge 1 commit into from

Conversation

opes
Copy link

@opes opes commented Sep 1, 2021

SafeSRC Backend Grading Notes

Note: Using the split view in the "Files Changed" tab of this PR will make it easier to see grading comments & suggestions.

DO NOT MERGE THIS PR

Notes

  • Solid tests!
    • For the ones that were skipped, I made adjustments and unskipped them to show how you can get them passing.
    • There was an extraneous test that was removed.
  • Code was formatted well overall; I made some suggestions for renaming variables & formatting.
    • Model folder was renamed to models, hence why git thinks the files were removed.
  • While the scraper works, it has a few issues:
    • There's a fair amount of duplicate code
    • It uses request, which has been deprecated, so I switched it to use node-fetch.
    • node-fetch uses Promises, which can be combined with .reduce to run the scraper sequentially and prevent overloading the database with too many connections.
  • Don't forget to include an .env-example file so other developers can know what needs to be set.
  • Make sure not to commit your .vscode folder

Great work, overall! This was a complex backend with a lot of different parts, but it's organized well which makes it easy to follow.

@opes opes marked this pull request as ready for review September 1, 2021 23:06
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.

1 participant