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: quickFix for enum, const, property #900

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

p-spacek
Copy link
Contributor

What does this PR do?

add quick fix code-action for enums, consts, properties

Screenshot 2023-07-10 at 13 56 00 Screenshot 2023-07-10 at 13 55 16

What issues does this PR fix or reference?

no ref

Is it tested? How?

tests

@coveralls
Copy link

coveralls commented Jul 10, 2023

Coverage Status

coverage: 84.247% (+0.07%) from 84.174%
when pulling ae9c4e9 on jigx-com:feat/quick-fix-enum-const-property
into f039273 on redhat-developer:main.

@@ -221,7 +225,7 @@ export class YamlCodeActions {
const results: CodeAction[] = [];
for (const diagnostic of diagnostics) {
if (diagnostic.code === 'flowMap' || diagnostic.code === 'flowSeq') {
const node = getNodeforDiagnostic(document, diagnostic);
const node = getNodeForDiagnostic(document, diagnostic);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just typo

endCharacter,
severity,
source,
code
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just code added to parameters

@@ -1385,6 +1387,7 @@ function validate(
length: propertyNode.keyNode.length,
},
severity: DiagnosticSeverity.Warning,
code: ErrorCode.PropertyExpected,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if it wouldn't be better to unify data.properties to data.values for the incorrect property

@p-spacek p-spacek force-pushed the feat/quick-fix-enum-const-property branch from 1bffa52 to d6718e4 Compare September 5, 2023 11:37
@p-spacek
Copy link
Contributor Author

p-spacek commented Sep 5, 2023

Hello @msivasubramaniaan, can I ask you for a review, please?

Array.isArray((diagnostic.data as YamlDiagnosticData).values)
) {
return (diagnostic.data as YamlDiagnosticData).values;
} else if (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this else really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean quickFix for properties? or can you explain your point further?

  • I think that property quickFix can be useful
  • note that there are different property names in diagnostic.data object (values vs properties)

for (const value of values) {
results.push(
CodeAction.create(
value,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This returns only the value which is not very descriptive of the operation. perhaps we can replace it with something like

Suggested change
value,
`Use value "${value}"`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have preferences here, I can remember that I tried a few combinations but I ended up with only {value}.
I was inspired by the English spelling checker extension (Code Spell Checker extension)
image

small disadvantage of Use value "${value}" is that this PR adds quickfix also for properties of the object (not value), so we need to display the text based on the error type

  • but for sure it's not a problem

so please make a decision

Copy link
Collaborator

@gorkem gorkem left a comment

Choose a reason for hiding this comment

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

Only a couple of minor comments. It also needs to be rebased.

gorkem

This comment was marked as duplicate.

@p-spacek p-spacek force-pushed the feat/quick-fix-enum-const-property branch from d6718e4 to 77bf8bc Compare October 11, 2023 11:42
@p-spacek
Copy link
Contributor Author

p-spacek commented Nov 3, 2023

@gorkem any updates here, please?
check updates and answers
Can you please rerun the jobs? It failed because of unavailable jsonSchemaStore

@p-spacek p-spacek force-pushed the feat/quick-fix-enum-const-property branch from 77bf8bc to 5baa96e Compare May 29, 2024 12:13
@msivasubramaniaan
Copy link
Contributor

@p-spacek Please upstream the branch

@p-spacek
Copy link
Contributor Author

p-spacek commented Oct 17, 2024

@p-spacek Please upstream the branch

@msivasubramaniaan I merged the main into this PR, but there is some schema change that caused the test to fail:

  -  "source": "yaml-schema: Composer Package"
  +  "source": "yaml-schema: Package"

UPDATE: I pushed the test fix for this schema rename

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.

4 participants