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
Obtenir les CGUs de plusieurs fournisseurs #16
Conversation
a94353a
to
172f5e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests fail at execution due to two different errors
- Repeated for all tests that try to commit:
Error: _commit for test_service_provider document terms_of_service experienced an error: MissingNameError: No name was provided for author in the argument or in the .git/config file.
- In
CGUs#updateTerms
:Error: ENOENT: no such file or directory, unlink 'cgus/test/data/sanitized/first_provider/tos.md'
.
General feedback
- I see the added value of logging what's going on but I'm wondering if it's not a bit too many
console.log
. It's in the code now and the messages have been properly written, so it's fine for me to leave them there, but I feel we should have a discussion around the logging interface and expectations 🙂 - I feel it might be worth adding ESlint considering that quite a few comments were around meaningless stuff such as semicolons or spacing 🙂
src/history/persistor.js
Outdated
|
||
if (!fsApi.existsSync(directory)){ | ||
if (!fsApi.existsSync(directory)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!fsApi.existsSync(directory)){ | |
if (!fsApi.existsSync(directory)) { |
src/history/persistor.js
Outdated
if (!fsApi.existsSync(directory)){ | ||
fsApi.mkdirSync(directory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it needs to exist before writing the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why not await
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I can do that, I didn't see that I already import fs.promises
It's weird for your tests, everything is ok on my machine. All tests pass. Do you have a |
I tried to add eslint but as we use |
Great! Thanks for this 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests pass after cp .env.example .env
… Sorry!
Obtenir les CGUs de plusieurs fournisseurs
Obtenir les CGUs de plusieurs fournisseurs
fixes #12