Skip to content

SOLR-14024 Invalid html generated by changes2html.pl#38

Merged
janhoy merged 1 commit intoapache:mainfrom
asalamon74:SOLR14024
Mar 24, 2021
Merged

SOLR-14024 Invalid html generated by changes2html.pl#38
janhoy merged 1 commit intoapache:mainfrom
asalamon74:SOLR14024

Conversation

@asalamon74
Copy link
Copy Markdown
Contributor

Original lucene-solr pull request: apache/lucene-solr#1060

Description

changes2html.pl generates invalid html

Solution

Fix it to generate a valid html.

Tests

Created the html and tested it using https://validator.w3.org/

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

Copy link
Copy Markdown
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

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

Manual review looks good.

Please indicate a quick way to test the change in practice, i.e. command to invoke the script and what to look for, i.e. can we run a tool on the resulting HTML file to see how the former style was broken and the new is ok?

Is there any visual change in the output, or just "correctness"?

Comment thread solr/CHANGES.txt
@asalamon74
Copy link
Copy Markdown
Contributor Author

I'm calling the following to invoke the script:

./gradlew changesToHtml

It generates the html which can be found in ./solr/documentation/build/site/changes/Changes.html.

I was using https://validator.w3.org/#validate_by_input as a validator a copied the generated html to it. For the original HTML it gave me a couple hundred errors like:

  • Self-closing syntax (/>) used on a non-void HTML element. Ignoring the slash and treating as a start tag.
  • Element ul not allowed as child of element ul in this context. (Suppressing further errors from this subtree.)
  • No li element in scope but a li end tag seen.

The solr/CHANGES.txt change is not unrelated. That empty line caused a problem in the generation (the script was starting a new block because of that).

Visually I tried to keep it as close as possible, there are minor changes, like marginsize.

@janhoy
Copy link
Copy Markdown
Contributor

janhoy commented Mar 23, 2021

Ok, tried it and it looks good. The one (three) errors still there is due to the huge license text comment which consumes >1024 chars, not sure if there is anything to do with that - actually I think we are allowed to use a smaller license text for files like this, see https://www.apache.org/legal/src-headers.html#is-a-short-form-of-the-source-header-available

"Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements; and to You under the Apache License, Version 2.0. "

Although I think this is not a valid case to use it. No big deal.

@asalamon74
Copy link
Copy Markdown
Contributor Author

Interesting, because I don't remember running into this license problem, but @rmuir mentioned a very similar thing in the lucene part of this change: apache/lucene#31

@janhoy janhoy merged commit f966d98 into apache:main Mar 24, 2021
@janhoy
Copy link
Copy Markdown
Contributor

janhoy commented Mar 24, 2021

Thanks András!

epugh pushed a commit to epugh/solr that referenced this pull request Oct 22, 2021
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.

2 participants