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.

Sign up for GitHub

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

Jump to bottom

docs: Don't show mobile TOC menu JS code on CLI man page #2639

Merged
merged 2 commits into from
Nov 12, 2022
Merged

docs: Don't show mobile TOC menu JS code on CLI man page #2639

merged 2 commits into from
Nov 12, 2022

Conversation

tmszi
Copy link
Member

@tmszi tmszi commented Nov 9, 2022

Fixes #2633.

@tmszi tmszi added bug Something isn't working backport_needed manual Documentation related issues HTML Related code is in HTML labels Nov 9, 2022
@tmszi tmszi added this to the 8.3.0 milestone Nov 9, 2022
@tmszi tmszi marked this pull request as draft November 9, 2022 12:21
@tmszi
Copy link
Member Author

tmszi commented Nov 9, 2022 •
edited
Loading

bs4 py module (BeautifulSoup class with decompose() method for remove HTML tag) in not part of Python standard lib.

@wenzeslaus
Copy link
Member

I'm happy to see this fix and I have couple things to ask for. Please, move the HTML validity and cleanup changes to a separate PR - that's easy to review and get in and it will simplify this PR. Generally, describe the proposed changes in the PR description.

For the new dependency, that would be better bot to backport unless it is made optional - it seems that adding a dependency for micro release is unexpected. Alternatively, do two separate PRs. Possible, although maybe not preferred scenario is a revert of the HTML changes causing #2633 in 8.2 and quick release of 8.3.

It's bad that the CI did not catch #2633 before the code was merged. In case bs4 is used, such test is perhaps not as needed as much.

@tmszi tmszi force-pushed the fix-man-page-js-script branch from 4fd485a to 1dc46be Compare November 9, 2022 19:27
@tmszi tmszi changed the title utils/g.html2man: remove HTML <script> element tag from HTML content string utils/g.html2man: don't show mobile TOC menu JS code on cli man page Nov 9, 2022
@tmszi
Copy link
Member Author

tmszi commented Nov 9, 2022

The simplest an quick solution with less code and a dependency on the external Python module bs4 and use one existed HTML parser class HTMLParser, commit 1dc46be.

@nilason
Copy link
Contributor

nilason commented Nov 9, 2022

This looks promising now ! Quick test seems to work fine.

@tmszi
utils/g.html2man/testsuite: add g.html2man module test
c98b8de 
Test the conversion of the module manual HTML page to the CLI manual
page.
@tmszi
Copy link
Member Author

tmszi commented Nov 10, 2022

A unit test has been added c98b8de.

@tmszi tmszi marked this pull request as ready for review November 10, 2022 11:57
@tmszi tmszi requested review from neteler and nilason November 11, 2022 02:54
nilason
nilason approved these changes Nov 11, 2022
View reviewed changes
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

Works great, + for the simple solution. Thanks!

wenzeslaus
wenzeslaus approved these changes Nov 11, 2022
View reviewed changes
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Short code, fixes a bug, and adds a test. What else can one ask for?

@tmszi tmszi merged commit 8a8bccc into OSGeo:main Nov 12, 2022
tmszi added a commit to tmszi/grass that referenced this pull request Nov 12, 2022
@tmszi
utils/g.html2man: don't show mobile TOC menu JS code on cli man page (O…
7b14579 
…SGeo#2639)
@tmszi tmszi deleted the fix-man-page-js-script branch November 12, 2022 03:25
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
@tmszi @ninsbl
utils/g.html2man: don't show mobile TOC menu JS code on cli man page (O…
2e77278 
…SGeo#2639)
marisn pushed a commit to marisn/grass that referenced this pull request Jun 2, 2023
@tmszi @marisn
utils/g.html2man: don't show mobile TOC menu JS code on cli man page (O…
ea625ab 
…SGeo#2639)
@wenzeslaus wenzeslaus changed the title utils/g.html2man: don't show mobile TOC menu JS code on cli man page docs: Don't show mobile TOC menu JS code on CLI man page Jun 6, 2023
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
@tmszi @neteler
utils/g.html2man: don't show mobile TOC menu JS code on cli man page (O…
bc8e54b 
…SGeo#2639)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wenzeslaus wenzeslaus wenzeslaus approved these changes

@nilason nilason nilason approved these changes

@neteler neteler Awaiting requested review from neteler

Assignees
No one assigned
Labels
bug Something isn't working HTML Related code is in HTML manual Documentation related issues
Projects
None yet
Milestone
8.3.0
Development

Successfully merging this pull request may close these issues.

[Bug] man pages corrupted
3 participants
@tmszi @wenzeslaus @nilason