Conversation
pdw207
left a comment
pdw207
left a comment
There was a problem hiding this comment.
Great Job! 😃👏{score: 7, comparative: 1.75 }
Some of my feedback related to the ES6 migration and other comments that might make sense to either include in this PR or not. I think it makes it much easier to follow.
| return hash; | ||
| } catch (e) { | ||
| throw new Error(e); | ||
| } |
There was a problem hiding this comment.
Can you remove the try catch blocks? I think you can simplify the nesting here and right now you are just throwing the error you just caught. Also, the nested catch, throw prepends an Error: in the message.
build/build.js
Outdated
| await fs.writeFileSync(RESULT_PATH, result); | ||
| return hash; | ||
| } catch (e) { | ||
| throw new Error(e); |
build/build.js
Outdated
| callback(null, hash); | ||
| }); | ||
| } | ||
| const finish = async hash => { |
There was a problem hiding this comment.
Suggest we rename function to something like writeJSON or writeEmojiHash and move the Complete: ${Object.keys(hash).length} entries. into this utility function. Then you don't even need to return the hash.
build/build.js
Outdated
| const build = async () => { | ||
| try { | ||
| let hash = {}; | ||
| hash = await processEmoji(hash); |
There was a problem hiding this comment.
Looks like you don't really need to pass hash into the processEmoji function. Suggest something like:
try {
const hash = await processEmoji();
await finish(hash);
} catch (e) {
console.error(e)
}
There was a problem hiding this comment.
You right, I didn't realize this.
lib/language-processor.js
Outdated
| if (!languages[languageCode]) { | ||
| // Try to load specified language | ||
| try { | ||
| const language = require('../languages/' + languageCode + '/index'); |
| .split(' '); | ||
| }; | ||
|
|
||
| module.exports = tokenize; |
There was a problem hiding this comment.
I'm not sure about this change. Why move the module.exports call down here?
There was a problem hiding this comment.
It follows the same style as other files. I will change if not approved.
There was a problem hiding this comment.
If there is only one exported function I think its fine to have it the assignment made along with the function declaration up top. That being said, lib/index.js should probably be done in the same way. @thisandagain should probably clarify.
There was a problem hiding this comment.
@thisandagain I think this just needs one more review before its ready to be merged.
package.json
Outdated
| "nlp", | ||
| "sentiment analysis" | ||
| ], | ||
| "keywords": ["sentiment", "analysis", "nlp", "sentiment analysis"], |
There was a problem hiding this comment.
Please don't make changes in the package.json that are simply aesthetic. It makes this diff hard to read / manage.
| process.stdout.write('IMDB accuracy: ' + validate(imdb) + '\n'); | ||
| process.stdout.write('Yelp accuracy: ' + validate(yelp) + '\n'); | ||
| process.stdout.write(`Amazon accuracy: ${validate(amazon)}\n`); | ||
| process.stdout.write(` IMDB accuracy: ${validate(imdb)}\n`); |
There was a problem hiding this comment.
It's just padding text for alignment. I will remove if not approved.
thisandagain
left a comment
thisandagain
left a comment
There was a problem hiding this comment.
Thanks! A few changes, but overall this looks really good.
|
Any updates on these merge conflicts? |
4 participants
Related to #136