Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Stem/lancaster stemmer #35

Merged
merged 2 commits into from
Oct 12, 2017
Merged

Stem/lancaster stemmer #35

merged 2 commits into from
Oct 12, 2017

Conversation

greenat92
Copy link
Collaborator

@greenat92 greenat92 commented Sep 4, 2017 •
edited

I'll inspire the implementation from nltk Lancaster stemmer.
#34

@greenat92 greenat92 mentioned this pull request Sep 4, 2017
Closed
kariminf
kariminf reviewed Sep 4, 2017
View reviewed changes
Copy link
Owner

@kariminf kariminf left a comment

Choose a reason for hiding this comment

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

Good
Just use double quotations because I configured codefactor to verify them 👍

@kariminf
Copy link
Owner

kariminf commented Sep 4, 2017

Sorry, I didn't notice you have a problem when adding a new function.

Apparently, the instructions outside describe are executed before the describes (the asynchronous mambo-jambo). Anyway, you can add an initialization function inside describe, like this:

//morpho.setCurrentStemmer("lancasterStemmer");
describe("English Morphology Lancaster Stemmer", function(){
    before(function(){
    morpho.setCurrentStemmer("lancasterStemmer");
  });
...

same thing for Porter:

describe("English Morphology porter stemmer ", function(){
    before(function(){
    morpho.setCurrentStemmer("porterStemmer");
  });

@greenat92 greenat92 changed the title [WIP]Stem/lancaster stemmer (Don't merge) Stem/lancaster stemmer Oct 12, 2017
@greenat92
Copy link
Collaborator Author

Salam @kariminf, ready to review

greenat92
greenat92 commented Oct 12, 2017
View reviewed changes
test/unit/eng.morpho_t.js
expect(morpho.stem("string")).to.eql("string"); // ditto 'string'
expect(morpho.stem("meant")).to.eql("meant"); // ditto 'meant'
expect(morpho.stem("cement")).to.eql("cem"); // ditto 'cem'
//expect(morpho.stem("ness")).to.eql("nest"); // Change s to t 'nest' TODO: Make it change s to t 'nest'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've made that as a todo in #12

greenat92
greenat92 commented Oct 12, 2017
View reviewed changes
test/unit/eng.morpho_t.js
//expect(morpho.stem("ness")).to.eql("nest"); // Change s to t 'nest' TODO: Make it change s to t 'nest'
});

/*it('Strip Prefixes', function(){ TODO: make it strip Prefixes
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've made that as a todo in #12

@kariminf kariminf merged commit 1ce1c9f into kariminf:master Oct 12, 2017
@kariminf
Copy link
Owner

Salam @LBenzahia
Thank you for the new function :)

I merged it if that's OK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kariminf kariminf kariminf approved these changes

Assignees
No one assigned
Labels
ready to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@greenat92 @kariminf