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.

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

fix test 206 #278

Merged
merged 1 commit into from
Aug 6, 2022
Merged

fix test 206 #278

merged 1 commit into from
Aug 6, 2022

Conversation

tatchi
Copy link
Collaborator

@tatchi tatchi commented Jul 31, 2022

Input:

[ΑΓΩ]: /φου

[αγω]

The current result in master:

File "tests/spec-206.html", line 1, characters 0-0:
diff --git a/_build/default/tests/spec-206.html b/_build/default/tests/spec-206.html.new
index 93c6540..97ea23b 100644
--- a/_build/default/tests/spec-206.html
+++ b/_build/default/tests/spec-206.html.new
@@ -1 +1,2 @@
-<p><a href="/%CF%86%CE%BF%CF%85">αγω</a></p>
+<p>[ΑΓΩ]: /φου</p>
+<p>[αγω]</p>

There are two issues here:

  1. Labels are not being matched, hence it is not recognized as a link. This is what I fixed in unicode link label normalization (fix test 539) #277. I included that change in the first commit.

  2. The url destination is not percent encoded. From the spec:

A link destination consists of either

  • a sequence of zero or more characters between an opening < and a closing > that contains no line endings or unescaped < or characters, or

  • a nonempty sequence of characters that does not start with <, does not include ASCII control characters or space character, and includes parentheses only if (a) they are backslash-escaped or (b) they are part of a balanced pair of unescaped parentheses. (Implementations may impose limits on parentheses nesting to avoid performance issues, but at least three levels of nesting should be supported.)

And `ASCII control characters are:

An ASCII control character is a character between U+0000–1F (both including) or U+007F.

I'm not sure why '\x80' .. '\x9F' were being matched since, according to the spec, they're not considered as an `ASCII control characters.
Removing them from the pattern fixes the test and doesn't break any other so I guess that's fine.

Fix #272

@tatchi tatchi force-pushed the fix-206 branch 2 times, most recently from db52f84 to 07fe574 Compare August 2, 2022 06:35
Copy link
Collaborator

@sonologico sonologico left a comment

Choose a reason for hiding this comment

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

I could see the normalization function being in a separate file so that we have parser.ml a bit more focused, but maybe it's not worth it for a single function.

@tatchi tatchi force-pushed the fix-206 branch 2 times, most recently from 03e6d09 to bf2be7f Compare August 5, 2022 06:04
@shonfeder
Copy link
Collaborator

That's a good idea, @sonologico. Perhaps we could followup with a refactor to clean up that massive file at some point soon :)

@shonfeder
Copy link
Collaborator

Will wait to review here until it's been rebased/updated to account for the work its based on.

@tatchi
Copy link
Collaborator Author

tatchi commented Aug 6, 2022

Will wait to review here until it's been rebased/updated to account for the work its based on.

It's rebased 😊

@shonfeder
Copy link
Collaborator

Perfect, surgical fix here. We’ll diagnosed! Thanks :)

@shonfeder shonfeder merged commit 60c12f0 into master Aug 6, 2022
@shonfeder shonfeder deleted the fix-206 branch August 6, 2022 12:04
mseri pushed a commit to ocaml/opam-repository that referenced this pull request Dec 13, 2022
CHANGES:

- Expose the HTML escape function `htmlentities` (ocaml-community/omd#295 @cuihtlauac)

- Support generation of identifiers in headers (ocaml-community/omd#294, @tatchi)

- Support GitHub-Flavoured Markdown tables (ocaml-community/omd#292, @bobatkey)

- Update parser to support CommonMark Spec 0.30 (ocaml-community/omd#266, @SquidDev)

- Preserve the order of input files in the HTML output to stdout (ocaml-community/omd#258,
  @patricoferris)

- Fix all deviations from CommonMark Spec 0.30 (ocaml-community/omd#284, ocaml-community/omd#283, ocaml-community/omd#278, ocaml-community/omd#277, ocaml-community/omd#269,
  @tatchi)
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.

Fix spec 206: case insensitive comparison
3 participants