Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop variant analysis from published pack if no language selected #3261

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

charisk
Copy link
Contributor

@charisk charisk commented Jan 22, 2024

Tiny change to stop processing a variant analysis using a published back if the user doesn't select a language.

Checklist

N/A - the functionality being changed is behind an unveiled feature flag:

  • 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.

@charisk charisk requested a review from a team as a code owner January 22, 2024 12:52
Copy link
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 with one comment just to clarify my own understanding of things

@@ -223,6 +223,9 @@ export class VariantAnalysisManager
});

const language = await askForLanguage(this.cliServer);
if (!language) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have any behavioural effect right now? We're using the default value of throwOnEmpty = true so it should throw a UserCancellationException if the user doesn't select a language? Is that how it works?

Regardless there are still some code paths where it still returns undefined, and I agree we should abort in this case.

Aborting without logging anything should be fine. As far as I can tell we'll always have already logged the reason when returning undefined from askForLanguage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this have any behavioural effect right now? We're using the default value of throwOnEmpty = true so it should throw a UserCancellationException if the user doesn't select a language? Is that how it works?

Ah yeah it looks like there is no behaviour change. That's how I'm reading the code too.

So this will mainly make the types a bit nicer.

@charisk charisk merged commit 9644e5d into main Jan 22, 2024
14 checks passed
@charisk charisk deleted the charisk/stop-if-no-language branch January 22, 2024 15:59
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.

2 participants