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

Fix ability to disable "no grammar" message #1024

Merged

Conversation

datho7561
Copy link
Contributor

@datho7561 datho7561 commented May 6, 2021

Fixes redhat-developer/vscode-xml#467

Signed-off-by: David Thompson [email protected]

@datho7561 datho7561 force-pushed the 467-fix-no-grammar-disable branch from 565d40e to c40f87f Compare May 6, 2021 14:42
@datho7561 datho7561 requested a review from angelozerr May 6, 2021 14:43
@fbricon
Copy link
Contributor

fbricon commented May 6, 2021

Can we have unit tests that check there won't be another regression in the future pretty please?

@datho7561 datho7561 force-pushed the 467-fix-no-grammar-disable branch from c40f87f to e36dbe5 Compare May 6, 2021 14:57
@datho7561
Copy link
Contributor Author

I don't know how to effectively unit test this scenario, but I attached an attempt

@datho7561 datho7561 force-pushed the 467-fix-no-grammar-disable branch 2 times, most recently from c01164b to a653c63 Compare May 6, 2021 15:25
@datho7561 datho7561 force-pushed the 467-fix-no-grammar-disable branch 2 times, most recently from ee9c570 to 1a8c030 Compare May 6, 2021 16:05
Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

The change definitely fixes things, but just a question of fixing the testcase.

@datho7561 datho7561 force-pushed the 467-fix-no-grammar-disable branch 3 times, most recently from ba8ea79 to 00ee4bc Compare May 6, 2021 19:38
@datho7561 datho7561 force-pushed the 467-fix-no-grammar-disable branch from 00ee4bc to 506c401 Compare May 6, 2021 20:44
@rgrunber
Copy link
Contributor

rgrunber commented May 6, 2021

I would just restore it to how it was before with the manual setting of methods and calling merge. No need for setAccessible(..). IMO, I think it's fine to merge.

Anyone adding to validation settings should add to that testcase and they would catch any issues with merge.

@datho7561 datho7561 merged commit 74cfa7a into eclipse-lemminx:master May 6, 2021
@datho7561 datho7561 deleted the 467-fix-no-grammar-disable branch May 6, 2021 20: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.

"xml.validation.noGrammar" no longer works
3 participants