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

mustache_lint: Wrap stray tags into valid parent. #195

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

mustache_lint: Wrap stray tags into valid parent. #195

wants to merge 2 commits into from

Conversation

kabalin
Copy link
Member

@kabalin kabalin commented Feb 21, 2019

This fix is partially addressing MDL-56504 by adding an extra wrappers to tags that have strict parent requirement according to the spec (e.g. it is invalid to use <li>...</li> without wrapping it <ul>...</ul> tag).

Bats run output:
If only test (ac593e9) is applied:

ci:mustache-lint-partials-stray-tags-fix> bats tests/1-mustache_lint.bats 
 ✓ mustache_lint: Good mustache file
 ✓ mustache_lint: No example content
 ✓ mustache_lint: Example content invalid json
 ✓ mustache_lint: Mustache syntax error
 ✓ mustache_lint: HTML validation issue
 ✓ mustache_lint: Partials are loaded
 ✗ mustache_lint: Partial contains elements with strict parent requirement
   (from function `assert_success' in file tests/libs/bats-assert/src/assert.bash, line 114,
    in test file tests/1-mustache_lint.bats, line 111)
     `assert_success' failed
   
   -- command failed --
   status : 1
   output (5 lines):
     Validating using https://html5.validator.nu
     Running mustache lint from f851201f58662264ad7864dbbf8e2240ce7bf84a to de7907ebe5594bac7cea77c2bf847ea7c4554999:
     /home/ruslan/git/ci/moodle/lib/templates/linting.mustache - WARNING: HTML Validation error, line 2: Stray start tag “td”. (ad><body><td>Hello )
     /home/ruslan/git/ci/moodle/lib/templates/linting.mustache - WARNING: HTML Validation error, line 2: Stray end tag “td”. (ello World</td></bo)
     Mustache lint problems found
   --
   
 ✓ mustache_lint: Full HTML page doesn't get embeded in <html> body
 ✓ mustache_lint: Theme templates load theme partials
 ✓ mustache_lint: Test quote and uniq helpers are working
 ✓ mustache_lint: Test eslint doesn't run when not present
 ✓ mustache_lint: Test eslint runs when npm installed

when feature patch (8523227) is applied:

ci:mustache-lint-partials-stray-tags-fix> bats tests/1-mustache_lint.bats 
 ✓ mustache_lint: Good mustache file
 ✓ mustache_lint: No example content
 ✓ mustache_lint: Example content invalid json
 ✓ mustache_lint: Mustache syntax error
 ✓ mustache_lint: HTML validation issue
 ✓ mustache_lint: Partials are loaded
 ✓ mustache_lint: Partial contains elements with strict parent requirement
 ✓ mustache_lint: Full HTML page doesn't get embeded in <html> body
 ✓ mustache_lint: Theme templates load theme partials
 ✓ mustache_lint: Test quote and uniq helpers are working
 ✓ mustache_lint: Test eslint doesn't run when not present
 ✓ mustache_lint: Test eslint runs when npm installed

12 tests, 0 failures

@kabalin
mustache_lint: Test partials with stray tags.
ac593e9 
Stray tags are those that got strict parent requirement, e.g. you can't
use <li>...</li> without wrapping it in <ul>...</ul> tag.
@kabalin
mustache_lint: Wrap stray tags into valid parent.
8523227 
Attempt to fix a partial template containing elements with a strict
parent requirements.
@kabalin
Copy link
Member Author

kabalin commented Feb 21, 2019

Thinking if it needs some watchdog for while loops count? (if someone add a new elements mapping that would point to each other, this may produce an infinite loop) :/

@danpoltawski
Copy link
Contributor

I like the concept of this solution to avoid more false positives (the the primitive html wrapping was always a bit of a ugly hack).

Though worth noting that a proper declarative metadata type solution might have some value and was how I intended to get this fixed: https://tracker.moodle.org/browse/MDL-56504 It would help the template library too

@kabalin
Copy link
Member Author

kabalin commented Mar 6, 2019

Yes, to be honest Dan I found MDL-56504 ticket when the code for this solution was ready, thus decided to create PR anyway. Need to make it a habit to search tickets before starting writing anything 😄

@stronk7
Copy link
Member

stronk7 commented Mar 22, 2019 •
edited
Loading

Hi,

sorry for the delay... somehow I got this by email but ended well down in my lists!

Just thinking if, maybe... both solutions could coexist. In this way:

  1. Nearly sure (contradict me if I'm wrong!)... any of the cases detected by the new wrap_content() function... are partials, correct? Or maybe they don't need to be? In any case, this is secondary, lets continue...

  2. We also can make a pass over all the .mustache files looking for ({{>) included partials, yes?

  3. When we know that something is a partial, because of 2):

    • If it matches 1), we still can HTML validate it.
    • If it doesn't matches 2), we skip HTML validation (noticing it).

  4. If @ MDL-56504 there is someday an agreement about how to tag partials... we can start cross-checking those tags with the information we have about partials (detect cases missing the tag... or the opposite, tagged templates that are perfectly valid as main ones as they are.

Not sure about the exact way to put everything working together, just drafting how they may work (the clever wrapping, the detected partials used, the html validation and the future partials tag).

@stronk7 stronk7 force-pushed the master branch 4 times, most recently from 2524a78 to 44c50d6 Compare November 10, 2020 23:17
@stronk7 stronk7 force-pushed the master branch 6 times, most recently from 975e856 to 2a863f0 Compare June 4, 2022 16:33
@stronk7 stronk7 force-pushed the master branch 4 times, most recently from 7c835b9 to 0126fe3 Compare March 9, 2023 17:38
@brendanheywood
Copy link

So other than a rebase and dust the cobwebs off, whats left to get this fixed?

@stronk7
Copy link
Member

stronk7 commented Aug 24, 2023

Cross-linking with https://tracker.moodle.org/browse/MDLSITE-6744, for the records.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kabalin @danpoltawski @stronk7 @brendanheywood