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

83 project set up scripts arent os agnostic #96

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ZacharyLeahan
Copy link
Collaborator

I added "npm run restore-test-data-win" as "npm run restore-test-data" did not run in Windows.
"npm run restore-test-data-win" is os agnostic as it relies on a js script. This is tested on my windows machine.

This pull request is for the js script, package.json reference, and readme update.

Evidence of this running locally:
image
image

@ZacharyLeahan ZacharyLeahan self-assigned this Feb 11, 2024
@ZacharyLeahan ZacharyLeahan linked an issue Feb 11, 2024 that may be closed by this pull request
@ZacharyLeahan ZacharyLeahan linked an issue Feb 11, 2024 that may be closed by this pull request
Copy link
Collaborator

@boutell boutell left a comment

Choose a reason for hiding this comment

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

The request for changes is mostly about the minor confusion in the README, my comment about your promise chain based code is more about taking it in a direction you'll find a lot easier to maintain in the long run but it's up to you.


```
sudo mongod --dbpath ~/data/db
mongosh mongodb://localhost:27017/pa-wildflower-selector
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't run an instance of mongodb, you already did that by setting it up to "install mongod as a service." This opens the mongo shell, which is nice but not required for anything we do below I think. You could just say "for Windows, this happened automatically when you installed mongod as a service."

@@ -0,0 +1,98 @@
const https = require('https');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is impressive, but you're working way harder than you have to here by using promise chains (calling then() and catch() functions in chains etc). Using promise constructors and chains is not necessary anymore. You should switch this up to use async/await so your code can read as a simple linear series of steps. It's a lot easier to maintain.

There's a pretty good tutorial here:

https://javascript.info/async-await

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.

Update README for windows and mac DB restore scripts is nix specific (not OS agnostic)
2 participants