Skip to content
This repository has been archived by the owner on Aug 16, 2023. It is now read-only.

Issue 51: Adding updatedAt and labels to the output #53

Merged
merged 4 commits into from
Oct 30, 2020

Conversation

redgardner
Copy link

Updated the graphQL query to grab createdAt and all labels (I defaulted to 100, but didn't do another page grab). Not sure if y'all want to limit the number of labels fetched or not, but I would be worried if a ticket had more than 100.

Updated the output to include and array of strings for the labels (with just the label name) and the createdAt field

Copy link
Collaborator

@kevindigo kevindigo left a comment

Choose a reason for hiding this comment

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

Thanks for putting this up!

I'll take a closer look at this tomorrow, but one thing I know we'll want is some README updates describing the change. It should describe the format of the output, and call out the 100 label limit.

Copy link
Collaborator

@kevindigo kevindigo left a comment

Choose a reason for hiding this comment

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

It's looking good. I had a couple suggestions/requests, and a couple questions.

src/gitHubIssueFetcher.ts Outdated Show resolved Hide resolved
src/issueFinder.ts Outdated Show resolved Hide resolved
src/issueFinder.ts Outdated Show resolved Hide resolved
src/gitHubIssueFetcher.ts Outdated Show resolved Hide resolved
src/issueFinder.ts Outdated Show resolved Hide resolved
@redgardner
Copy link
Author

Thanks for the review. Just pushed some changes

Copy link
Collaborator

@kevindigo kevindigo left a comment

Choose a reason for hiding this comment

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

The changes look good, and I ran it locally with the correct results. 👍
Please fix up the linting issues, and then I'll merge it.

}

interface GitHubRepository {
nameWithOwner: string;
}

export interface GitHubLabelEdge {
node: {
name: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just ran npm run lint and it pointed out some missing semi-colons and extra whitespace. We should probably mention that in our README.

Copy link
Author

Choose a reason for hiding this comment

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

I'll run and add to the readme.

Happy to open another ticket to make it so that linting runs in pre-push hooks. I was actually thinking about linting earlier and then forgot when I went to commit

Copy link
Collaborator

Choose a reason for hiding this comment

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

For other projects, I have used a pre-commit hook for linting. I like catching that as early in the process as possible. We used husky. If you could create a ticket, that would be much appreciated. Thanks!

@redgardner
Copy link
Author

Ran linting. Added info to it to the README. Let me know if there's anything else outstanding!

@kevindigo kevindigo linked an issue Oct 30, 2020 that may be closed by this pull request
@kevindigo kevindigo merged commit 2d48332 into indeedeng:master Oct 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Save issue Labels with issues returned by IssueFinder.findIssues
3 participants