Skip to content

Add cutoff repos and counts to error message#1408

Merged
robertbrignull merged 1 commit intomainfrom
robertbrignull/cutoff_repos
Jun 28, 2022
Merged

Add cutoff repos and counts to error message#1408
robertbrignull merged 1 commit intomainfrom
robertbrignull/cutoff_repos

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

The queries API is changing so that it won't fail when you try to run a query on more than 1000 repos. Instead it'll cap the repos off at 1000 and tell you in the response which repos weren't queried (up to a limit of 100 repos).

This PR is for handling that change in the API and display the information in the log message. While there I also updated the log message in general to include the counts of the repositories in the various states. I think this makes it more consistent if we're including the count for the cutoff repositories case.

Any input on the wording of the log message is very welcome. Writing these messages is very hard 😢

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.

@robertbrignull robertbrignull requested review from a team as code owners June 28, 2022 11:22
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Messages look good to me!

Copy link
Copy Markdown
Contributor

@elenatanasoiu elenatanasoiu left a comment

Choose a reason for hiding this comment

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

This looks great 👍

I've left a suggestion, but feel free to ignore it as I think the way we do message building is fine.

if (response.errors.private_repositories?.length) {
logMessage += `${eol2}Non-public repositories:${eol}${response.errors.private_repositories.join(', ')}`;
if (private_repositories?.length) {
logMessage += `${eol2}${private_repositories.length} repositories are not public:${eol}${private_repositories.join(', ')}`;
Copy link
Copy Markdown
Contributor

@elenatanasoiu elenatanasoiu Jun 28, 2022

Choose a reason for hiding this comment

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

Since we're building a log message from:

  • two end of lines
  • a number
  • a message
  • another eol
  • a list

in three separate situations, extracting those lines into a method might improve readability.

e.g.

logMessage += buildCutOffMessage(private_repositories, "repositories are not public");

let buildCutOffMessage = (list, message) => `${eol2}${list.length} ${message}:${eol}${list.join(', ')}`; 

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I like this suggestion but I think I'll just get the PR merged first so I can get this and the API change done today. Then potentially come back to this and see how it works out. Definitely the potential to make the area more understandable, but also could spend a lot of time messing with it.

@robertbrignull robertbrignull merged commit 628e0e9 into main Jun 28, 2022
@robertbrignull robertbrignull deleted the robertbrignull/cutoff_repos branch June 28, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants