Skip to content

Sort Gist files by user-defined sort order#1891

Merged
koesie10 merged 1 commit intomainfrom
koesie10/sort-gist-files
Dec 22, 2022
Merged

Sort Gist files by user-defined sort order#1891
koesie10 merged 1 commit intomainfrom
koesie10/sort-gist-files

Conversation

@koesie10
Copy link
Copy Markdown
Member

This will sort the files in an exported Gist by the user-defined sort order. It does so by prefixing the files with result-{index}- where the index is the 1-based index of the repository in the sort order. It will automatically pad the index with leading zeros to ensure that the files are sorted in the correct order.

Unfortunately, we can't just use {index}- because numbers sort before the _ character, which is used in the summary filename to place it first.

There are also some changes in how we determine which repositories to export since we need to know in advance how many zeroes we need to pad the index with. There should be no functional changes in which repositories are actually exported.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

This will sort the files in an exported Gist by the user-defined sort
order. It does so by prefixing the files with `result-{index}-` where
the `index` is the 1-based index of the repository in the sort order.
It will automatically pad the index with leading zeros to ensure that
the files are sorted in the correct order.

Unfortunately, we can't just use `{index}-` because numbers sort before
the `_` character, which is used in the summary filename to place it
first.

There are also some changes in how we determine which repositories to
export since we need to know in advance how many zeroes we need to pad
the index with. There should be no functional changes in which
repositories are actually exported.
@koesie10 koesie10 marked this pull request as ready for review December 21, 2022 09:48
@koesie10 koesie10 requested a review from a team as a code owner December 21, 2022 09:48
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

LGTM

I agree this should have no effect on behaviour. We were previously yielding repos with no results but then they got filtered out later. Now those repos are filtered out earlier in the process.

repoStates.find((r) => r.repositoryId === repo.repository.id)
?.downloadStatus ===
VariantAnalysisScannedRepositoryDownloadStatus.Succeeded,
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we incorporate the extra array pass into the original filtering, perhaps in a separate new function (e.g. filterSuccessfulReposWithResults)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think that would be really specific to this use-case (especially with the repoStates being in a different array), so I think this wouldn't be clearer if we split it out into a separate function which is only used once and depends on some specific input.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's quite long to parse when you're trying to work out what it's doing, so I was mostly looking for some sort of way to make it easy to read the next time we come to it. But it's not a blocking issue.

@koesie10 koesie10 merged commit be79d68 into main Dec 22, 2022
@koesie10 koesie10 deleted the koesie10/sort-gist-files branch December 22, 2022 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants