Skip to content

Conversation

@AndrewGable
Copy link
Contributor

Pls review - @tgolen

Applies feedback from here: #415 (review) (sorry it's a bit late!)

Fixed Issues

Fixes N/A

Tests

Verify GitHub actions passes ✅

@AndrewGable AndrewGable requested a review from tgolen November 17, 2020 00:04
@AndrewGable AndrewGable requested a review from a team as a code owner November 17, 2020 00:04
@AndrewGable AndrewGable self-assigned this Nov 17, 2020
@botify botify requested review from Dal-Papa and removed request for a team November 17, 2020 00:04
Dal-Papa
Dal-Papa previously approved these changes Nov 17, 2020
tgolen
tgolen previously approved these changes Nov 17, 2020
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Great, thanks! Those descriptions on the tests really help. We still have an unresolved discussion about the config files, so I'll leave this open for a bit longer depending on how that conversation is resolved.

@AndrewGable AndrewGable dismissed stale reviews from tgolen and Dal-Papa via 391957e November 17, 2020 18:01
const path = require('path');
const dotenv = require('dotenv');

const env = dotenv.config({path: path.resolve('./.env.example')}).parsed;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't exactly sure what .env file would be best, but I thought this one fit the best.. Let me know if you disagree @tgolen

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I actually hadn't considered the .env.example file. I was thinking maybe we would need a .env.testing file, but I think using the example file is brilliant 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think using path can be overkill sometimes. I'm pretty sure this could be path: './.env.example'. I'm sure this was just from copy/pasting what we have elsewhere, but I would also say the same thing about wherever you copied it from.

const path = require('path');
const dotenv = require('dotenv');

const env = dotenv.config({path: path.resolve('./.env.example')}).parsed;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think using path can be overkill sometimes. I'm pretty sure this could be path: './.env.example'. I'm sure this was just from copy/pasting what we have elsewhere, but I would also say the same thing about wherever you copied it from.

@tgolen tgolen merged commit 543d841 into master Nov 19, 2020
@tgolen tgolen deleted the andrew-test-changes branch November 19, 2020 18:03
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