Fixes #15: Added new file sentiment-analysis.js. Added dependencies #46
Conversation
|
@jatinAroraGit can you rebase this please? |
src/sentiment-analysis.js
Outdated
| /* This file contains the code for analyzing blog posts text to identify the | ||
| * negative or positve words being used in a post and return a summary of it | ||
| * along with a score. The file uses a node module called sentiment to implement | ||
| * the functionality of analyzing text of blogs. The function accepts striped |
There was a problem hiding this comment.
Typo on line 4: "The function accepts striped" should be "The function accepts stripped"
Otherwise looks good 👍 , need to rebase to match the current master branch.
There was a problem hiding this comment.
Hey @ImmutableBox ,
Resolved this in commit 259b31c
|
Hey there, |
src/sentiment-analysis.js
Outdated
| * negative or positve words being used in a post and return a summary of it | ||
| * along with a score. The file uses a node module called sentiment to implement | ||
| * the functionality of analyzing text of blogs. The function accepts stripped | ||
| * HTML text as parameters i.e text containg no tags, and returns a promise |
There was a problem hiding this comment.
What does "stripped HTML" mean? Do you want plain text or HTML? If the former, I'd say that instead.
src/sentiment-analysis.js
Outdated
| * object whcih contains the result | ||
| */ | ||
|
|
||
| var Sentiment = require('sentiment'); |
src/sentiment-analysis.js
Outdated
| */ | ||
|
|
||
| var Sentiment = require('sentiment'); | ||
| var sentiment = new Sentiment(); |
There was a problem hiding this comment.
Prefer const here too, and also, let's move this into your function below so we don't reuse this between runs. If there's any shared state between runs, we'll get into weird bugs.
src/sentiment-analysis.js
Outdated
| var sentiment = new Sentiment(); | ||
|
|
||
| module.exports.startAnalysys = function(blogText){ | ||
| var result; |
There was a problem hiding this comment.
This can be simplified bit:
return Promise.resolve(sentiment.analyze(text));Since this call doesn't throw, we just need to wrap it in a call to resolve
Adds async function. Co-Authored-By: David Humphrey <david.humphrey@senecacollege.ca>
|
Sorry @jatinAroraGit going to have to rebase because #55 was merged. Should only need to fix |
src/sentiment-analysis.js
Outdated
| */ | ||
|
|
||
| module.exports.run = async function (text) { | ||
| const Sentiment = require('sentiment'); |
There was a problem hiding this comment.
This line needs to go above: we only want to require this once, not every call to run(). The next line, where you create an instance, is what I wanted in here.
There was a problem hiding this comment.
This is still not fixed. Move line 10 out of this function, and above like you had it before.
birtony
changed the title
update dependency prettier to v2.5.1
Fixes #15
The pull request will add the feature of analyzing sentiment of a blog post with the help of a node package called sentiment.
Let me know if there are any issues with it.