Feature: zero return status code when all errors & warnings have been muted #933
Comments
|
On 3/31/21, Lucas Cimon wrote:
Calling HTML Tidy on this file:
```
$ tidy -quiet -lang en -indent -wrap 0 -modify index.html
line 13 column 5 - Warning: <div> proprietary attribute
"aria-valuetransitiongoal"
Could it be possible to add an option in
[htmltidy.conf](http://api.html-tidy.org/tidy/tidylib_api_5.4.0/tidy_quickref.html)
to ignore this kind of situation please?
The ability to suppress messages already exists.
--mute-id yes to see the id and then
--mute "id" to suppress the message
I' would like the `tidy` CLI to exit with status 0 while validating this
file.
I think that was an earlier feature request - if a message is
suppressed don't set the error status if that error/warning is found.
I don't remember it going anywhere tho :(
$ tidy -q -indent -w 120 --mute "PROPRIETARY_ATTRIBUTE" x.html
<!DOCTYPE html>
<html lang="fr">
<head>
<meta name="generator" content="HTML Tidy for HTML5 for Cygwin
version 5.7.35.next.2020.10.04">
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Issue reproduction for HTML Tidy</title>
</head>
<body>
<div class="progress">
<div class="progress-bar progress-bar-primary six-sec-ease-in-out"
role="progressbar" aria-valuetransitiongoal=
"80">
80%
</div>
</div>
</body>
</html>
$ echo $?
1
Even though no warnings were displayed you still get a non-zero exit code :(
Regards,
Lee
|
|
#794
where the resolution was
In the circumstances where tidy is being used in a toolchain, I would suggest that
it is not as simple as testing if the exit code is non-zero... If you have no interest
in warnings then you need to test if the exit is not 2, or negative... ie 0 or 1 is ok...
continue processing...
I don't use tidy in a toolchain, so it's an N/A for me.. but I can see
where it would be nice if the exit code was zero for no warnings
(including the case where all warning msgs were suppressed) and set if
there were warnings displayed.
Regards,
Lee
|
Lucas-C
changed the title
|
I renamed this issue and opened PR #934 to implement this. Also, I'd be willing to add a GitHub Actions pipeline to this project:
|
|
@Lucas-C, thank you for the issue, and the PR #934 to implement it... and @ler762, thanks for the comments, and links... At first glance it seems a reasonable Although there may be a precedent, of sorts - running tidy with Note you have already done the I also wonder about issue #921, PR #931... some changes in the exit code, in certain conditions... but maybe this does not change anything here... appreciate feedback, checking, comments,... thanks... As commented in #794, the present docs uses displaying... so as part of this PR, it would also be better to enhance the mute , and the Likewise, maybe the warn-proprietary-attributes doc should be considered... and I am sure there are other documentation issues... especially with the simple concept - A Not sure what Github Actions you are proposing... but we have resisted some So there seems a few points to clear up. Look forward to further feedback, comments, ideas, etc... thanks... |
|
Thank you for the detailed feedback @geoffmcl.
In my experiences with linters, this is a very common behaviour: csslint, eslint, hadolint pylint... They all provide an "ignore rule" mechanism, that silence the rule(s) specified, and set the final error code to zero if there are no errors detected, or only ones matching the silenced rules.
Thank you for bringing Outside the scope of this PR, my take on this would be to deprecate dedicated options like
I'm totally willing to add tests using
Similarly, after reading through them, IMHO they do not seem directly related to this matter.
I totally agree. I'll add a commit to this PR to update the docs once this change is agreed on.
Agreed. Maybe the best thing to do would be to expand on the existing doc at Running Tidy > Running Tidy in Scripts detailing exit codes with a mention of the "mute" logic. Regarding
OK. I'll leave that outside this PR. I'm curious to know why resisting automated tests pipelines though: is there an issue somewhere related to the subject, where I could find some background context? As a side note: I definitively had issues accessing the documentation at https://api.html-tidy.org/tidy/tidylib_api_next/ Either the website was really slow to answer or it even sometimes failed to display the page entirely. |
|
One more question : should I update a |
|
@Lucas-C, thank you for the further feedback... I will try to answer each of your perceived points, but may miss some... such in-line commenting is difficult... but... Re: What others do! While it may be a valid point, listing what others do, it does not carry that much weight. If I searched around I could probably find others that don't offer a generic And on checking some you mentioned, there seem some difference between the ideas Would you want to added ignore switches to the W3C validator? - it does have a filter - maybe 49:51 against... How can you indicate And maybe it comes down to examining the specific cases. While it might be ok if the warning is just a minor case - of course difficult to categorize into Of course, if it is a case that the particular warning/error has been subsequently approved, and I thought I remember some general check for hyphenated attributes, but can't find it just now... but it does seem there could be a case for a generic Re: generic vs dedicated, and depreciation Only since tidy As a start maybe the Re: regression test - as stated, add test, or not... not the point... Re: improve docs - that is a forever, ongoing task - will always be true! ;=)) And the Exactly how we are going to document this drastic change in the exit code, should be part of the approval case... simple, it can not be merged without docs... suggestions, samples, ideas, PR, welcome... Re: CI - Not against it - just present a case of why/what/when/..., and how this helps tidy, you, me, users, et al... pros and cons... and it could be considered. Do not have time to research all earlier comments, but one of the first seems bottom of 69, and #269, with many other refs... and others... - but not the point here... Re: side note - CI in others - I think I have heard that Re: CHANGELOG.md - Tidy does not have a change log, per se - the release notes 5.6 are generated from the What about the Agree 100% - focus on usability for users - that's what Hmmm, did I cover everything? Probably not, but ask again if I missed... Be aware, this discussion, so far, has pushed me more towards the negative ;=)) IMHO: tidy should exit: Look forward to further feedback, comments, doc change suggestions, patches, PR, etc, etc, ... thanks... |
|
@Lucas-C, my continued exploration... Re: depreciation of an option Thought I found that we do have a diff --git a/src/config.c b/src/config.c
index bae56b8..bb0da4e 100644
--- a/src/config.c
+++ b/src/config.c
@@ -293,6 +293,7 @@ static const struct {
ctmbstr name; /**< name of the deprecated option */
TidyOptionId replacementId; /**< Id of the replacement option, or 0 if none. */
} deprecatedOptions[] = {
+ { "warn-proprietary-attributes", TidyMuteReports },
{ NULL }
};BUT this is not really a See the code... particularly note This seems clearly a case of a mis-leading table name... should be, say For a Maybe could be added, changed... Oh, well... Re: Change exit code But for the moment, on this issue of As indicated, the simple documentation that - Like, yeah, but it is not And maybe even One problem I have with And there has been no documentation change Continuing investigation... look forward to further feedback, ideas, patches, PR, etc, etc... thanks... |
|
Hi @geoffmcl Thank you for your detailed answers.
Alright. All CI pipelines engines that I have used (GitHub Actions, Gitlab CI, Travis CI, Circle CI...) use a fail-fast approach when executing scripts at each stage, similarly to Currently, a user of
If you say so. As far as I'm concerned, this isssue and PR #934 can be closed. |
|
@Lucas-C , don't be discouraged. Sorry for the delay piping in. My suggestion would simply be to add a new option that works exactly like This also has the potential to allow us to deprecate some of the existing configuration switches. Personally, I prefer to have switches be specific configuration options, because this makes it simple to automate in Tidy GUI applications, and you don't have to hunt down the awkward configuration option id with As for the deprecation methods @geoffmcl was looking at, the header indicates: The In general, I would prefer to have a callback handle this deprecation, though. This would move the logic from the library to the consumer, e.g., the console application. In principle, the library should have a discrete set of options that affect behavior of the Tidy'ing process, with no duplicates and no redundancies, and output filtering should be handled by the client (e.g., console application). As a library, it still offers the convenience of parsing options and files, of course. This represents a fairly extensive re-write of the console application, however, and so we usually just keep piling stuff into the library. As for CI, I've added automated testing and brought the regression tests back into this repo. |
|
Related problem: Any warnings contribute to a non-zero exit code, even when That flag results in a program console trace with an exit code that can prevent the rest of a well formulated Please ensure that |
|
Just to add another example… I am preparing html files used within a web app. The requirement is that the files have empty title tags. They are filled in later with js I guess. I'd love to be able to get a 0 exit code when I have muted the empty title tag warning. |
Hi!
Thank you for maintaining this robust HTML linter!
I'm using bootstrap-progressbar on an old project,
that uses custom HTML attributes like
aria-valuetransitiongoal.There is a minimal HTML usage example (without the JS libs):
Calling HTML Tidy on this file:
Could it be possible to add an option in htmltidy.conf to ignore this kind of situation please?
I' would like the
tidyCLI to exit with status 0 while validating this file.The text was updated successfully, but these errors were encountered: