Merged
Conversation
This adds the new `stargazers_count` and `updated_at` fields in the repositories to the appropriate `gh-api` and `shared` types. To make testing easier this also moves the `variant-analysis-processor.test.ts` to the pure tests since it doesn't and shouldn't depend on any `vscode` APIs.
It seems like the Storybook stories were not being type-checked by CI and got out-of-sync with the required types. This fixes the types and also uses the factories to reduce the chance of this happening with future changes.
This will add the star count and last updated fields to the repository row. We are able to re-use some components from remote queries, but we cannot re-use `LastUpdated` since it requires a numeric duration, while we are dealing with an ISO8601 date.
This uses a script to add the new `stargazers_count` and `updated_at` to the scenario files. This is done by using the GitHub API to get the information for each repo and then updating the scenario file. The `updated_at` values are not completely representative since they are the `updated_at` at time of running the script, rather than at the time the variant analysis was run. However, this should not really matter in practice. An alternative for scanned repositories might be getting the creation time of the `database_commit_sha` commit.
The tests were expecting the wrong results, except for the case where the time was less than a second. For less than a second ago, it makes sense to return "this minute". For times that are 2.001 minutes ago, it makes sense to return "2 minutes ago" rather then the previous behaviour of "3 minutes ago".
robertbrignull
approved these changes
Nov 2, 2022
Contributor
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
I haven't gone over every line of code change with a fine toothed comb, but I've looked through quickly and the code changes look fine to me. I've also tried it out locally, checking the storybooks, and trying running a real variant analysis and a mocked analysis and everything works.
@charisk, I don't know if you want to look over the scripts / scenarios changes? I don't have much context on those yet.
elenatanasoiu
approved these changes
Nov 3, 2022
Contributor
elenatanasoiu
left a comment
elenatanasoiu
left a comment
There was a problem hiding this comment.
This is awesome! 🙌
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please review this PR commit-by-commit. There were some missing types on the stories, so to properly use Storybook those needed to be fixed, resulting in a large diff. Only the first and third commit are relevant for the repository metadata display.
Checklist
ready-for-doc-reviewlabel there.