Conversation
zeke
left a comment
zeke
left a comment
There was a problem hiding this comment.
Thanks for adding this! I made a few suggestions, and let's get some test coverage on it.
index.js
Outdated
|
|
||
| fromFile (filename) { | ||
| let rawdata = fs.readFileSync(filename) | ||
| let glossary = JSON.parse(rawdata) |
There was a problem hiding this comment.
If we're only supporting JSON, this could just a be a require instead.
index.js
Outdated
| let rawdata = fs.readFileSync(filename) | ||
| let glossary = JSON.parse(rawdata) | ||
| for (var i in glossary) { | ||
| assert(i && i.length, `term is required for description ${glossary[i]}`) |
There was a problem hiding this comment.
Rather than re-doing this validation, we should just use the existing .add() method and get the existing validation for free.
readme.md
Outdated
| ### `glossary.fromFile(filename)` | ||
|
|
||
| Uploads entries from the a file provided. Entries only exist in memory until you | ||
| call `glossary.upload()` but file remains unchanged. |
There was a problem hiding this comment.
"but file remains unchanged." doesn't seem necessary.
readme.md
Outdated
| Uploads entries from the a file provided. Entries only exist in memory until you | ||
| call `glossary.upload()` but file remains unchanged. | ||
|
|
||
| - `filename` String (required) |
There was a problem hiding this comment.
Might want to mention that this should be a full path.
|
Hey @maddhruv wanna take another look at this when you get a chance? |
|
@zeke I would surely make relevant changes today |
|
@zeke have a look at the changes made |
|
Looking good. Can you add some tests for this new functionality? |
|
the build is failing as I have referred a non-existing dummy file so should I also add the dummy file in the repo? |
Yeah this is typically referred to as a "test fixture". It can be included in the repo itself in a file like |
|
That's a good start. You'll also want to test the "happy path" scenario, where the API works as expected. Try using this command to help ensure you have good test coverage: (We should probably also add this command to the npm scripts for convenience) |
|
I am poor at testing! |
|
@zeke can this be merged now?
|
☝️ @maddhruv I'd love to see this implemented before we land it. |
|
|
You have a test that verifies the behavior when the "wrong thing" happens (empty file), but no test that verifies when the "right thing" happens (non-empty file). This is sometimes called the "happy path", meaning everything works as expected. You should add a test that has a non-empty JSON file and verify that the |
|
😸 |
| assert(term, 'Term or description not found') | ||
| this.add(term[0], term[1]) | ||
| }) | ||
| this._entries = glossary |
There was a problem hiding this comment.
this._entries = glossary is not necessary. This line can be removed.
| let glossary = require(filename) | ||
| assert(Object.keys(glossary).length, 'The file seems to be empty') | ||
| glossary.forEach(term => { | ||
| assert(term, 'Term or description not found') |
There was a problem hiding this comment.
assert is not necessary. This is already handled by the add method.
| @@ -0,0 +1,3 @@ | |||
| { | |||
There was a problem hiding this comment.
The docs and the implementation expect an array of arrays, not an object.
| @@ -0,0 +1,4 @@ | |||
| { | |||
There was a problem hiding this comment.
This should also be an array, not an object.
| }) | ||
|
|
||
| test('upload from a file', () => { | ||
| expect(() => { |
There was a problem hiding this comment.
This should be something like:
glossary.fromFile('./test/glossary_test.json')
expect(glossary.entries.length).toEq(2)2 participants
Function to add entries from a JSON file