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

feat: fetch feature flag for Code Consistent Ignores [IDE-274] #455

Merged
merged 12 commits into from
May 13, 2024

Conversation

cat2608
Copy link
Contributor

@cat2608 cat2608 commented May 7, 2024

Description

  • This change executes Language Server command to fetch Snyk Code Consistent Ignore flag

This is the foundation work for the Code details panel dynamic rendering. When enabled, Code details will render HTML provided by LSP

Checklist

  • Tests added and all succeed
  • Linted
  • CHANGELOG.md updated
  • README.md updated, if user-facing

Screenshots / GIFs

@cat2608 cat2608 changed the title feat: add Snyk Code feature flag to settings and configuration [IDE-274] feat: fetch feature flag for Code Consistent Ignores [IDE-274] May 12, 2024
@cat2608 cat2608 marked this pull request as ready for review May 12, 2024 20:22
@cat2608 cat2608 requested a review from a team as a code owner May 12, 2024 20:22
@@ -25,6 +27,9 @@ class ConfigurationWatcher implements IWatcher {
constructor(private readonly logger: ILog) {}

private async onChangeConfiguration(extension: IExtension, key: string): Promise<void> {
if (key === ADVANCED_ORGANIZATION_SETTING) {
await configuration.fetchAndSetFeatureFlag(FEATURE_FLAGS.consistentIgnores);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we not make this call for ADVANCED_ORGANIZATION so we don't need to add a new configuration key? We basically want to update the feature flag whenever the org changes right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is for the settings page.

Is there another way to listen for a organisation change?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm saying that I think ADVANCED_ORGANIZATION is already meant to represent a change for that panel: https://github.com/snyk/vscode-extension/blob/main/src/snyk/common/constants/settings.ts#L19. It looks like we're not listening to this in onChangeConfiguration though, for some weird reason.

What is the difference between ADVANCED_ORGANIZATION_SETTING and ADVANCED_ORGANIZATION?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I'm sorry! You were saying that I duplicated a variable!
You are right! I didn't realise I duplicated snyk-advanced.organization.
There is no difference, I made a mistake declaring a variable that already exists.
Deleting it now, thank you.

Copy link
Contributor

@teodora-sandu teodora-sandu left a comment

Choose a reason for hiding this comment

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

Tested it and it works! Thanks

@cat2608 cat2608 merged commit 1d2b3b7 into main May 13, 2024
7 checks passed
@cat2608 cat2608 deleted the feat/add-ff-for-code-ignores branch May 13, 2024 14:13
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