Feat: add any-of-labels option #319
Conversation
There was a problem hiding this comment.
Hello and it's nice to see a contribution!
I have some suggestions (didn't want to add a request changes):
- the README.md is missing some update as well regarding the table containing all the options
- eventually you could also add at the end of the README.md an example regarding this new option to point out the diff with
only-labels - it would be better with some coverage to handle at least:
- the case were it's empty
- the case were there is a label but not matching the one in the issue
- the case were there is a label matching the one in the issue
- the case were there is multiple labels and none match the one in the issue
- the case were there is multiple labels and one match the one in the issue
| import {DefaultProcessorOptions} from './constants/default-processor-options'; | ||
| import {generateIssue} from './functions/generate-issue'; | ||
|
|
||
| describe('any-of-labels option', () => { |
There was a problem hiding this comment.
I like the fact that you decided to put them in a dedicated file
|
|
||
| await sut.processIssues(); | ||
|
|
||
| expect(sut.staleIssues).toHaveLength(1); |
There was a problem hiding this comment.
I saw that the other tests are not using this method to check the length yet it's the most appropriate one ❤️
There was a problem hiding this comment.
Yeah I'm still not 100% satisfied with that one cause there is some implementation detail leaking out. It would probably be worth it to explore adding a custom matcher for this in the future, maybe? Like expect(sut).toHaveProcessedIssues().
| }); | ||
| }); | ||
|
|
||
| class IssuesProcessorBuilder { |
There was a problem hiding this comment.
I prefer to have all the "tests arrange" in the top of the file.
It makes more sense to me since it's the first thing to code and the first thing executed as well in the tests.
What do you think?
There was a problem hiding this comment.
Well, ideally that fixture would be moved into a separate file and enriched to be shared and reused by all tests. But even if it stays here, I don't really see much value having it on top in this case because 1) the test arrangement would be the instantiation of the builder, not its declaration, 2) the builder itself, albeit useful, is not that interesting, and anyone going through the test suite should be able to get what it does just by looking at how it's used tbh, and 3) having the describe/test blocks as close to the beginning of the file as possible is more practical, IMO.
| private _issues: Issue[]; | ||
|
|
||
| constructor() { | ||
| this._options = {...DefaultProcessorOptions}; |
There was a problem hiding this comment.
Why not assign the properties on creation to reduce the class size instead of using a constructor?
There was a problem hiding this comment.
Ah, a OO purist would say the constructor is the place to do it! lol. But really, I'm in the habit of doing it like this for more practical reasons: 1) if you add some more fields, and some of them are initialized via a constructor arg, and others are initialized inline where they are declared, that could get a bit disorganized, 2) it gives a good indication for potential refactorings, e.g. those default options could be injected, instead of imported (imagine we have some other predefined defaults, for example).
But it's just a matter of preference, really. I'm not opposed to getting rid of the constructor 😀.
__tests__/any-of-labels.spec.ts
Outdated
| } | ||
|
|
||
| issues(issues: Partial<Issue>[]): IssuesProcessorBuilder { | ||
| this._issues = issues.map((issue, idx) => |
There was a problem hiding this comment.
| this._issues = issues.map((issue, idx) => | |
| this._issues = issues.map((issue, index): Issue => |
__tests__/any-of-labels.spec.ts
Outdated
| this._options, | ||
| idx, | ||
| issue.title || 'Issue title', | ||
| issue.updated_at || '2000-01-01T00:00:00Z', // we only care about stale/expired issues here |
There was a problem hiding this comment.
Using a spread operator would be cleaner IMO.
You can just merge the default values with the given issue.
What do you think?
There was a problem hiding this comment.
Sure, that could work. I saw you had that function to generate the mock issues and thought it would be frowned upon if I didn't use it lol. I'm a fan of fluent builder patterns for this type of setup, maybe adding a IssueBuilder in the future can be useful 😉.
Co-authored-by: Geoffrey Testelin <[email protected]>
|
@joeveiga I gave you some conflicts 😛 |
|
I can merge this when the conflicts are resolved |
|
I merged a bunch of PRs and will release a new release after all of them have run on master (and possibly one or two more to merge). Probably end of this week and I will cut a release. |
#302