-
Notifications
You must be signed in to change notification settings - Fork 148
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
feat: add support for plural form #37
Conversation
Add the support for the plural forms of contributions (e.g: `docs`, `tests`, `plugins`, ...)
Codecov Report
@@ Coverage Diff @@
## master #37 +/- ##
=======================================
Coverage 59.57% 59.57%
=======================================
Files 4 4
Lines 94 94
Branches 12 12
=======================================
Hits 56 56
Misses 33 33
Partials 5 5 Continue to review full report at Codecov.
|
@AllContributorsBot please add @jakebolam for infras, code and docs |
^ Just to test if it works on here (other than on the CLI). |
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.
I think shouldn't commit package-lock.json
file.
src/utils/parse-comment/index.js
Outdated
let contributions = doc | ||
.match('#Contribution') | ||
const contributions = doc | ||
.normalize({ plurals: true }) |
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.
Can we enable the test for this:
https://github.com/all-contributors/all-contributors-bot/blob/master/test/utils/parse-comment/index.test.js#L72
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.
Will do
src/utils/parse-comment/index.js
Outdated
const contributions = doc | ||
.normalize({ plurals: true }) | ||
.words() | ||
// .match('#Contribution') |
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.
Any reason for dropping 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.
I tried keeping it but since words like "tests" and any of the plural forms of validContributionTypes
get dropped after going through match('#Contribution')
.
.normalize({ plurals: true }) | ||
.words() | ||
// .match('#Contribution') | ||
.slice(5) | ||
.data() | ||
.map(data => { |
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.
Could you explain whats going on here?
Looks like this changes the behaviour to treat any word after the first couple as a contribution (excluding and).
Before behaviour was to find any word which could be a contribution (this also ensures the contributions are valid for the generator) and add them (meaning sentence structure was less important). e.g. @bot please add someone for blah blah
, @bot add someone for contributing doc and infra
, Jane you're crushing it in documentation and infrastructure, let's add Jane for her contributions. cc @bot
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.
Yeah, admittedly that solution is more of a temporary one (until something better and more in line with the match(...)
based approach is found.
None of the solutions where I kept match('#Contribution')
resolved in acknowledging contributions that were in the plural form.
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.
I'll have a play too, see if I can provide some more constructive feedback.
This work you've done well help alot!
@Berkmann18 recently switched the behaviour so it would show as a tag (e.g. need to use @all-contributors). See https://github.com/all-contributors/all-contributors-bot#adding-contributions |
I could not determine your intention. Basic usage: @all-contributors please add jakebolam for code, doc and infra We had trouble processing your request. Please try again later. |
Add the support for the plural forms of contributions (e.g: `docs`, `tests`, `plugins`, ...)
Okay, I've seen there's a change in master so after pushing the rebased version, I'll try again. |
So i'll make the contributing docs better (mention how we use yarn etc), but in the mean time you can get the bot running locally with The probot docs are pretty good, basically: https://probot.github.io/docs/development/#running-the-app-locally |
.match('#Contribution') | ||
.data() | ||
.map(data => { | ||
// This removes whitespace, commas etc | ||
let type = data.normal | ||
if (type !== 'ideas' && /s$/.test(type)) type = type.slice(0, -1) //-> singular |
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.
I think this falls down in a number of cases too (e.g. ing plurals, documenting/documentation/docs). If we were writing something to account for 1000s of words I'd say we keep down this road.
As we are only dealing with around 20 base terms, I think it far more accurate to just account for the variations ourselves: https://github.com/all-contributors/all-contributors-bot/pull/53/files#diff-b5ed782af77775c7d732fcefd331dd6cR39
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.
Yeah true.
Closing as it's now been done in #53. |
This PR adds the support for plural contributions such as
docs
,tests
,plugins
, ...Resolves #15