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

Suppress validation kinds based on file pattern #1275

Closed
merks opened this issue Aug 16, 2022 · 25 comments · Fixed by #1290
Closed

Suppress validation kinds based on file pattern #1275

merks opened this issue Aug 16, 2022 · 25 comments · Fixed by #1290
Assignees
Labels
enhancement New feature or request validation
Milestone

Comments

@merks
Copy link

merks commented Aug 16, 2022

Is there a way to opt of validation for specific know content types?

The question arises relates to this issue raised for PDE:

eclipse-pde/eclipse.pde#294

PDE has abused the XML Schema namespace for its own purpose leading to all kinds of validation errors that are not really errors for PDE's actual use of *.exsd resources.

Similarly a PDE *.target file doesn't specify a schema, but the editor knows the correct structure so a warning about the schema being missing is just noise to the user.

The Lemminx functionality is generally very cool and very useful, but it seems the only way to disable this for PDE's resources is to disable it in general, which definitely not desired either...

@angelozerr
Copy link
Contributor

angelozerr commented Aug 16, 2022

Is there a way to opt of validation for specific know content types?

No, the only thing that you can do is to disable lemminx validation (but globally)

image

In LemMinX side, content type is unkwown, only file extension can be used for that.

The question arises relates to this issue raised for PDE:

eclipse-pde/eclipse.pde#294

Ok thanks for the information. If I understand correctly you need to ignore some validation errors for a specific file extension (.exsd). But what about the granularity of this validation ignore for all .exsd files:

  1. ignore all validation (syntax + from XSD rules)
  2. ignore validation for XSD rules but for for syntax
  3. ignore some error code (ex : src-annotation)

I think the first (ignore all validation for all .exsd files) is doable easily. The others are possible, but it need some investigation.

@merks what do you prefer between 1, 2 or 3?

Similarly a PDE *.target file doesn't specify a schema, but the editor knows the correct structure so a warning about the schema being missing is just noise to the user.

I agree with you, and you can disable that with:

image

But it will disable validation that for all XML files which is shame, because this warning give the capability to provide some quickfix to generate the XSD, DTD from the given XML.

According to your 2 comments we should give the capability to ignore some error code for

@merks
Copy link
Author

merks commented Aug 16, 2022

Firstly, thanks so much for such a timely response!! 🥇

Is there a way to opt of validation for specific know content types?

No, the only thing that you can do is to disable lemminx validation (but globally)

image

In LemMinX side, content type is unkwown, only file extension can be used for that.

Yes, that's understandable...

The question arises relates to this issue raised for PDE:

eclipse-pde/eclipse.pde#294

Ok thanks for the information. If I understand correctly you need to ignore some validation errors for a specific file extension (.exsd). But what about the granularity of this validation ignore for all .exsd files:

1. ignore all validation (syntax + from XSD rules)

2. ignore validation for XSD rules but for for syntax

3. ignore some error code (ex : src-annotation)

I think the first (ignore all validation for all .exsd files) is doable easily. The others are possible, but it need some investigation.

@merks what do you prefer between 1, 2 or 3?

A *.exsd file definitely does not conform to the XML Schema 1.0 specification. I think they might have based the syntax on some earlier version of XML Schema before the 1.0 spec was final, but regardless, PDE does all its own validation so I think choice 1. is the most appropriate and it sounds like the simplest as well, so double goodness.

Similarly a PDE *.target file doesn't specify a schema, but the editor knows the correct structure so a warning about the schema being missing is just noise to the user.

I agree with you, and you can disable that with:

image

I think most users won't be so bothered by this warning but it would definitely be good to provide an ignore as way to disable this! Thanks quite a common choice in other contexts, such as Java checks...

But it will disable validation that for all XML files which is shame, because this warning give the capability to provide some quickfix to generate the XSD, DTD from the given XML.

According to your 2 comments we should give the capability to ignore some error code for

If we could ignore missing grammars that's a nice to have. Disabling all validation for *.exsd, I think is more crucial for plugin developers...

Thanks again for your openness!

@angelozerr
Copy link
Contributor

Firstly, thanks so much for such a timely response!!

You are welcome! And I'm very embarassed that LemMinX is annoying for PDE, so we need to fix that.

PDE does all its own validation so I think choice 1. is the most appropriate and it sounds like the simplest as well, so double goodness.

Ok we can start to implement a validation filter which enable/disable validation for a given pattern. The question after that is which plugins will set that in LemMinX? @mickaelistria @vrubezhny I think we could initialize this future setting in WWD to start.

If we could ignore missing grammars that's a nice to have.

We should do that with validation filter like we did for symbol https://github.com/redhat-developer/vscode-xml/blob/main/docs/Symbols.md#xmlsymbolsfilters

here a sample:

"xml.validation.filters": [
   // Disable all validation (syntax, etc) for *.exsd file since PDE manages the validation
   {
      "pattern": "*.exsd"
   },
   // Disable only noGrammar error code for .target file
   {
      "pattern": "*.target",
      "errorCodes": [
          "noGrammar"
      ] 
   }
]

@rgrunber
Copy link
Contributor

I think it makes sense to be able to filter out validation errors, by their error code based on file extension. I'm sure we may have cases of developers using a certain schema that isn't followed exactly (ie. want to ignore certain errors).

@merks
Copy link
Author

merks commented Aug 16, 2022

Cool idea! 🆒

@angelozerr
Copy link
Contributor

Cool idea!

Glad you like it. I will work on this issue.

When this settings will be available, it should be filled in Eclipse IDE to manage custom behavior for *.exsd and *.target

"xml.validation.filters": [
   // Disable all validation (syntax, etc) for *.exsd file since PDE manages the validation
   {
      "pattern": "*.exsd"
   },
   // Disable only noGrammar error code for .target file
   {
      "pattern": "*.target",
      "errorCodes": [
          "noGrammar"
      ] 
   }
]

The question is who should fill this settings? Ideally it should be the PDE plugins which should do that, but I fear is that it will take some time again to provide the fix with validation.

I think we should start step by step:

  1. step 1 : WWD hard code the settings xml.validation.filters with *.exsd and .target when XMLlanguage server is started
  2. step2: WWD provides an UI to customize xml.validation.filters to add/remove file pattern and filter some error code.
  3. step3: WWD provides an extension point to contribute to the settings. PDE will define the extension point to defines xml.validation.filters with *.exsd and .target

@merks @mickaelistria what do you think about that? @mickaelistria are you OK to hard code in WWD (step1) the settings for .exsd and.target?

@merks
Copy link
Author

merks commented Aug 18, 2022

@angelozerr To me this sounds like a good plan! Other "PDE" files such as *.product, category.xml, feature.xml, and plugin.xml also complain about missing grammars directly in their own structured editors which just looks weird.

@akurtakov Long term, will the platform be willing to use such an extension point? I believe it could do so without adding an actual dependency...

@akurtakov
Copy link
Contributor

As Lemminx is integrated in the IDE through WWD it makes sense to go the simplest path and disabling it there. If/when WWD provides such extenstion point I don't see issue for PDE using it to define this filter, though I don't see what's the benefit from hardcoding the filter in WWD without the extra work.

angelozerr added a commit to angelozerr/lemminx that referenced this issue Aug 18, 2022
@mickaelistria
Copy link
Contributor

Wouldn't it be more profitable to try improving/fixing the PDE xsd files so the .exsd file ? If most of things are a find/replace appInfo->appinfo, then it seems that it's simpler than adding extensions and customization here and there and would lead PDE to higher quality.
As for the behavior when no schema is defined, I agree it shouldn't be a warning by default. Is this something LemMinX should change or something we should change in Wild Web Developer?

@merks
Copy link
Author

merks commented Aug 20, 2022

It's not so simple to evolve *.exsd. There are many general things that XML Schema allows that make no sense in PDE restricted use case, but maybe that's not a big deal I guess. There are a substantial number of things used in ways that are wrong and cannot easily be made right, e.g., "qname" references don't respect namespace xmlns qualification rules. So evolving this it seems more of an ocean-boiling exercise with no substantial profit for the developers needing to do the work, nor profit for the consumers needing to do more work; in the end, it will work just the same as before, which is very nicely. After all, PDE has very good validation already so I'm not sure that any improvements need be made in validation, other than to not confuse users with pointless errors.

@mickaelistria
Copy link
Contributor

OK, so maybe the .exsd shouldn't be registered as being subtypes or XML, or maybe they should simply not reference the XMLSchema namespace. I'm not really found of the idea of implementing a lot of customization in multiple layers just to workaround the fact that 1 specific consumer (PDE) is actually pretending it's XML when it's not.

@merks
Copy link
Author

merks commented Aug 20, 2022

Given it's XML content, it wold be kind of odd at this point to change the content type definition to make it no longer XML just to disable errors from something that may or may not be installed anyway. Using a different namespace would have been better in hindsight, but doing so now is also trying to boil the ocean. What was proposed here seems useful in general and doesn't require changes in the entire ecosystem.

@mickaelistria
Copy link
Contributor

What was proposed here seems useful in general and doesn't require changes in the entire ecosystem.

I disagree that enabling complexity to workaround incorrect files is useful in general: it's extra development (that is not involved in better things) that would be beneficial for a small part of the ecosystem (the ecosystem of lemminx is the whole Java world, not only the Eclipse one) and would even kind of encourage people to stick to incorrect things.
Removing the xmlns instructions from the .exsd files would be easy (less "boiling the ocean" than requiring functional changes in LemMinX, then Wild Web Developer, then PDE...), and would keep things relatively correct.

@mickaelistria
Copy link
Contributor

I'm closing it here. There is really no need to change LemMinX here, even if one wants to implement some workaround. Wild Web Developer would be the most suitable place for a hack.

@angelozerr
Copy link
Contributor

I agree it shouldn't be a warning by default. Is this something LemMinX should change or something we should change in Wild Web Developer?

The change must be done in WWD with default preferences. But if you do that you will loose quickfix which generate xsd or dtd and update the xml with grammar association.

I planed to work on this filter like I explained below in order to fix all issues for PDE but @mickaelistria if you think you dont need this filter I will give up this implementation.

Please tell me if you need this filter validation because today I have the impression that PDE users hate LemMinX and I would like to avoid having this feeling.

@merks
Copy link
Author

merks commented Aug 20, 2022

The proposal here sounds constructive, helpful, and generally useful, but if you think it's best handled by WWD then there is an issue there now.

@merks
Copy link
Author

merks commented Aug 20, 2022

@angelozerr Thanks again for the help. I'm not sure I understand "closing" the issue without discussion and without creating a followup issue, but hey...

@rgrunber
Copy link
Contributor

I think it makes sense to be able to filter out validation errors, by their error code based on file extension. I'm sure we may have cases of developers using a certain schema that isn't followed exactly (ie. want to ignore certain errors).

Taking a step back from the WWD use case, is it worth while to have validation messages of certain types being suppressed ? @angelozerr If so, then let's at least keep this issue open, mark as an enhancement and rename it something more generic (eg. Suppress validation kinds based on file pattern). We can set the priority lower, since there's other things that probably need more attention, but at least users can vote on this.

@angelozerr angelozerr changed the title Opting out for specific known content types Suppress validation kinds based on file pattern Aug 23, 2022
@angelozerr angelozerr reopened this Aug 23, 2022
@angelozerr
Copy link
Contributor

Taking a step back from the WWD use case, is it worth while to have validation messages of certain types being suppressed ? @angelozerr If so, then let's at least keep this issue open, mark as an enhancement and rename it something more generic (eg. Suppress validation kinds based on file pattern). We can set the priority lower, since there's other things that probably need more attention, but at least users can vote on this.

Done.

@mickaelistria @merks I repeat me but if you need this feature for Eclipse IDE, please add a comment on this issue in order to I work on it.

I would like to avoid that Eclipse user hates LemMinx for this validation problem.

@mickaelistria
Copy link
Contributor

I think adding this feature would be counter-productive for LemMinX and even for PDE. There are other ways to avoid the warnings when editing .exsd files (fixing the .exsd file by making them as conform as possible to namespace, or by removing the namespace definition as it's not accurate; excluding .exsd files from LemMinX analysis/assistance with some new configuration capabilities in Wild Web Developer...).
The problem is not LemMinX, which is doing a great job at reminding there are bugs here and there, so I don't think LemMinX should host a solution (which would actually be a workaround).
Moreover, if you start making it simple for users to ignore some validation errors, then more and more users will use it and basically the resulting XML will be of lower quality. I don't think LemMinX has for goal to enable lower quality XML, does it?

@angelozerr
Copy link
Contributor

The problem is not LemMinX, which is doing a great job

Thanks!

I understand your point of view @mickaelistria but main idea with validation filter is to customize the severity (error, warning, ignore) for a given error code for a given pattern file. With ignore severity user will able to ignore the noGrammar (for plugin.xml for instance) and keep noGrammar waring for other XML files.

As @merks said, the exsd doesn't respect the full specification of XML Schema since so many years, I fear that it will not change and today I think users hates LemMinx to report this kind of error, but it is just my feeling.

@angelozerr angelozerr added this to the 0.22.0 milestone Aug 31, 2022
@angelozerr angelozerr self-assigned this Aug 31, 2022
@angelozerr
Copy link
Contributor

angelozerr commented Aug 31, 2022

@merks after some discussion with my team, we decide to provide this validation filter. To have the same behavior with other IDE/editor like vscode, sublime etc, we decide to provide a default value for this validation filter:

  • for exsd files (ignore the validation)
  • for plugin.xml, .classpath, .project, keep the validation but ignore the warning noGrammar which is displayed on the root document element.

It means that WWD will just include the future release of LemMinX jar without coding something to benefit with this validation filter.

@merks
Copy link
Author

merks commented Aug 31, 2022

@angelozerr Thanks! 🚀

angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 2, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 2, 2022
@angelozerr
Copy link
Contributor

Just for your information it starts working #1290 (comment)

angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 10, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 10, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 12, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 12, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 12, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 12, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 12, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 12, 2022
angelozerr added a commit to angelozerr/vscode-xml that referenced this issue Sep 12, 2022
angelozerr added a commit to angelozerr/vscode-xml that referenced this issue Sep 12, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 12, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 12, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue Sep 12, 2022
angelozerr added a commit to angelozerr/vscode-xml that referenced this issue Sep 12, 2022
@datho7561 datho7561 added enhancement New feature or request validation labels Sep 12, 2022
datho7561 pushed a commit that referenced this issue Sep 12, 2022
angelozerr added a commit to angelozerr/vscode-xml that referenced this issue Sep 12, 2022
@angelozerr
Copy link
Contributor

Thanks @datho7561 for your great review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request validation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants