Merge commits from validator-nu branch to master (then delete validator-nu branch)

[sideshowbarker]

I’d like to merge everything from the validator-nu branch to master, then delete the branch.

And then as far as the HTML checker, the parser integration for it could be built from the master branch here rather from from the validator-nu branch it has been getting built from.

So, looking at https://github.com/validator/htmlparser/compare/validator-nu, there are 19 commits in the validator-nu branch which aren’t in master. None have any merge conflicts with master.

(The validator-nu branch did previously have 20 commits; the additional commit was 2bbdc4d, which removed all the code for the XOM API — but that commit’s no longer in the branch.)

Among those 19 commits, 15 are changes to the error-reporting behavior of the parser.

And among those 15, the largest set are changes necessary to make the error-reporting behavior conform to current requirements in the HTML spec; those 7 commits are the following:

e03e56e Require UTF-8
9ce4bd4 Conform ampersand-error reporting to HTML spec
29a2645 Make consecutive hyphens in comments a non-error
f738d97 Drop parse error for missing end tag
1cd256d Emit error (not warning) for HTML4/XHTML1 doctype
da6c1ec Report error always for Transitional doctype
1ae6c63 Support “generate all implied end tags thoroughly”

The second-largest set of those error-reporting-behavior commits are 5 bug fixes:

003ad33 Fix typo in error message about over-deep tree
ef15099 Fix "non-space characters insided a table" typo
de301bc Fix grammar problem in HTML parser error message
f4f266c Correct error for EOF in “in template” state
7528857 Report 1024 as byte limit for meta charset sniff

So I think all 12 of those commits listed above could be considered necessary changes.

The remaining 3 changes to the error-reporting behavior, although not strictly necessary, are nonetheless pretty clearly improvements:

143a10b Improve message: bad start tag in noscript in head
5a209fd Remove warning about comments before doctype
5c8fe7a Stop reporting HTML4-specific parse errors

Then, along with the 15 changes above to the error-reporting behavior, there’s 1 change to the core code that, from discussion in #10, it seems we already clearly know we want in master too:

1357528 Ensure every Locator is also a Locator2

And then the remaining 2 changes to the code are minor ones that we don’t necessarily need but that, since the work’s already been done, I guess we should go ahead and merge:

1c66c6f Ensure Java8-runnable code even if Java9-compiled
895f606 Add suppress "unchecked" for a TreeBuilder method

Lastly, there’s one commit that’s not a code change but that just checks support files into the repo:

c5d11f9 Add .classpath, .settings, .project, .mailmap

So while that last change isn’t strictly necessary, it’s still generally useful — or at least it us for anybody using Eclipse. And in fact, I guess the .settings bits of it are particularly useful in that they can help contributors adhere to a consistent coding style in additions/updates to the source.

[anthonyvdotbe]

1c66c6f Ensure Java8-runnable code even if Java9-compiled

Would you mind to explain the scenario this fixes? I'd like to understand why this change is needed.

c5d11f9 Add .classpath, .settings, .project, .mailmap

I agree with .mailmap, but I'd like to avoid the others:

• .classpath and .project are generated/managed automatically by Eclipse for Maven projects. So even if you don't use Maven yourself: just the fact that Eclipse detects a pom.xml file (which I'm planning to update as part of Publish as modular JAR(s) to Maven Central #14), will make it manage those files itself
• .settings contains preferences, some of which are derived by the IDE from the pom.xml as well. Nearly all others are related to style/formatting. However, w.r.t. formatting, I believe it would be better to add an .editorconfig file, which covers the basics such as indentation and EOL characters, and is supported in pretty much every editor. For detailed formatting/style conventions, I believe PR reviews would suffice.

[sideshowbarker]

1c66c6f Ensure Java8-runnable code even if Java9-compiled

Would you mind to explain the scenario this fixes? I'd like to understand why this change is needed.

The context is validator/validator#634. For a succinct description of the underlying cause, see jetty/jetty.project#3244 (comment)

c5d11f9 Add .classpath, .settings, .project, .mailmap
…

• .classpath and .project are generated/managed automatically by Eclipse for Maven projects. So even if you don't use Maven yourself: just the fact that Eclipse detects a pom.xml file (which I'm planning to update as part of Publish as modular JAR(s) to Maven Central #14), will make it manage those files itself

Yes, so I agree we don’t need those.

• .settings contains preferences, some of which are derived by the IDE from the pom.xml as well. Nearly all others are related to style/formatting.

Yes, those style/formatting ones are the ones we care about

However, w.r.t. formatting, I believe it would be better to add an .editorconfig file, which covers the basics such as indentation and EOL characters, and is supported in pretty much every editor.

It’s been desirable for us so far to enforce more than just the basics. The style/formatting settings in the .settings files came originally from @hsivonen to me years ago because I wanted to ensure the changes I made were consistent with what he was doing.

For detailed formatting/style conventions, I believe PR reviews would suffice.

Sure. But in that case what I would want to ask is that anybody doing reviews should check, as part of the review, that the code they are reviewing conforms to the same style and formatting rules that are formalized in those .settings files. So is there a good way to do that which doesn’t require having those files in the repo? Or is there some other tooling we could have in the repo that provides the same level of granularity for style/format checking as those files?

In the end, if we don’t agree on those files going into master, then I’m OK with that. It’d just be a bit disappointing to me if we lose any way to help automate enforcement of style and formatting.

[anthonyvdotbe]

The context is validator/validator#634. For a succinct description of the underlying cause, see eclipse/jetty.project#3244 (comment)

Thanks, I see now. But isn't it possible to update the build to use --release by now? Apparently it wasn't possible at the time due to Nashorn usage, but it seems Nashorn was eliminated in validator/validator@941c81a?

Sure. But in that case what I would want to ask is that anybody doing reviews should check, as part of the review, that the code they are reviewing conforms to the same style and formatting rules that are formalized in those .settings files. So is there a good way to do that which doesn’t require having those files in the repo? Or is there some other tooling we could have in the repo that provides the same level of granularity for style/format checking as those files?

One automated solution would be to use Checkstyle and integrate it in the Maven build. This is IDE-agnostic and would fail the build if the code style/format isn't compliant. But I'm not sure it's actually worth the effort, given that all current contributors seem to use Eclipse.

In the end, if we don’t agree on those files going into master, then I’m OK with that. It’d just be a bit disappointing to me if we lose any way to help automate enforcement of style and formatting.

I don't feel strongly about this, so I'm fine with bringing these files into master. I think all that is really needed, is org.eclipse.jdt.core.prefs though, and then only the org.eclipse.jdt.core.compiler.problem and org.eclipse.jdt.core.formatter properties.

[sideshowbarker]

The context is validator/validator#634. For a succinct description of the underlying cause, see eclipse/jetty.project#3244 (comment)

Thanks, I see now. But isn't it possible to update the build to use --release by now?

OK, thanks — yeah, I can no longer reproduce the failure after dropping 1c66c6f, rebuilding the checker with --release 8 with Java 13, and then testing the resulting jar with Java 8.

So I’ve removed 1c66c6f from the validator-nu branch.

I’ve also removed the c5d11f9 “Add .classpath, .settings, .project, .mailmap” change from the validator branch. I can add the .mailmap change to master in a separate change, and we can use a pull request to discuss if/how to bring the org.eclipse.jdt.core.prefs stuff to master.

So we’re down to 17 commits in the validator-nu branch to merge over.

[carlosame]

Commits e03e56e, 1cd256d, da6c1ec and 5c8fe7a may cause regressions for end users, couldn't they be made/kept configurable (the last commit removes configurability for DOCTYPE)?

For example, 1cd256d enforces behaviour which is intended for "conformance checkers" (see whatwg commit 31c20af) but could be undesirable for other use cases (or at least is not how it used to work).

The XML DocumentBuilder in my DOM implementation (css4j's "native DOM") has a

setStrictErrorChecking(boolean)

that toggles the document builder to a less/more strict behaviour (default is strict), perhaps something similar could be added here to allow non-UTF8, etc.

Also, deprecating public methods before removing them seems like a more sensible policy for a project which has been around for quite a few years and is at version 1.4 already (I mean it is not a 0.x thing).

[sideshowbarker]

Commits e03e56e, 1cd256d, and da6c1ec are all necessary for bringing the parser into conformance with the current HTML spec. The user contract for the parser has always been that it intends to conform to the current requirements in the HTML spec. So it’s not a regression if the user starts getting a new error message from the parser due to the spec requirements having changed and the parser therefore changing in response to the spec change.

We have never provided an option for users to opt-into non-conformance with the HTML spec.

5c8fe7a is the only change that’s not strictly required for conformance with HTML spec requirements. But that’s because it’s not related to requirements in the current HTML spec but instead to requirements in the HTML4 spec. IMHO there’s no longer a clear-enough user need for the behavior to justify keeping the code around. Keeping just contributes to additional code paths and complexity in the code, and it’s not clear there’s any longer enough user real-world benefit to justify the cost of that additional code complexity.

[carlosame]

Commits e03e56e, 1cd256d, and da6c1ec are all necessary for bringing the parser into conformance with the current HTML spec.

You are right, I assumed that err() was producing an exception which is not the case. Sorry for the noise.

We have never provided an option for users to opt-into non-conformance with the HTML spec.

My setStrictErrorChecking is useful to prevent certain DOM-level exceptions from being thrown, but apparently you have all your error detection logic at lower level.

I should not try to look at patches for code that I do not really want to read in full (I manage a project which has DOM stuff, so not a good idea).

Keeping just contributes to additional code paths and complexity

For 5c8fe7a, one possibility would be to produce a version 1.5 with the class and methods (that are to be removed) being deprecated, and then a 2.0 with the code finally removed.

Deprecating first is a good practice but is a bit of additional work, not sure if worth it.

[sideshowbarker]

For 5c8fe7a, one possibility would be to produce a version 1.5 with the class and methods (that are to be removed) being deprecated, and then a 2.0 with the code finally removed.

Deprecating first is a good practice but is a bit of additional work, not sure if worth it.

Well, It might be more worth it if we found there are other things we want to deprecate while we’re at it. I mean, as long as we were going to be making breaking changes and bumping the major version number, it’d make sense to see if there’s anything else that seems like it’s reached its end of life/utility.

[hsivonen]

Sorry about the extremely slow reaction. (Again.)

e03e56e Require UTF-8

r+

9ce4bd4 Conform ampersand-error reporting to HTML spec

I added a couple of notes about the ASCII alphanumeric case in the ambiguous ampersand case.

29a2645 Make consecutive hyphens in comments a non-error

r+ with the note that COMMENT_LESSTHAN appears to transition to itself explicitly. Why?

f738d97 Drop parse error for missing end tag

r+

1cd256d Emit error (not warning) for HTML4/XHTML1 doctype

r+

da6c1ec Report error always for Transitional doctype

r+

1ae6c63 Support “generate all implied end tags thoroughly”

r+

003ad33 Fix typo in error message about over-deep tree

r+ (Needs Gecko-side error message adjustment)

ef15099 Fix "non-space characters insided a table" typo

r+ (Needs Gecko-side error message adjustment)

de301bc Fix grammar problem in HTML parser error message

r+ (Needs Gecko-side error message adjustment)

f4f266c Correct error for EOF in “in template” state

r+

7528857 Report 1024 as byte limit for meta charset sniff

r+

143a10b Improve message: bad start tag in noscript in head

r+ (needs Gecko-side error reporting adjustment)

5a209fd Remove warning about comments before doctype

r+

5c8fe7a Stop reporting HTML4-specific parse errors

r+, but since this affects the public API it would be kinda nice to do a release without this, then declare 2.0, take this, and immediately do a 2.0 release. Considering how bad I've been at releases, not sure if holding anything back is worthwhile, though. Maybe if I really, really resolve to do a release in August... (I'll be even more unresponsive that usual in July.)

Perhaps it would be the clearest to land this but revert it for a release branch, when I actually get around to spinning a release for real.

1357528 Ensure every Locator is also a Locator2

r+

1c66c6f Ensure Java8-runnable code even if Java9-compiled

r+

895f606 Add suppress "unchecked" for a TreeBuilder method

r+

c5d11f9 Add .classpath, .settings, .project, .mailmap

r+

Thanks. I'll prepare a corresponding Gecko patch queue for these.

[hsivonen]

e03e56e Require UTF-8

This one doesn't change any m-c-side code and is ready to be merged right away.

[hsivonen]

003ad33 Fix typo in error message about over-deep tree

This one can be fixed in Gecko independently of this merge, as the change is not in generated code.

[hsivonen]

This one can be fixed in Gecko independently of this merge, as the change is not in generated code.

I take that back, I misread what file changed.