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

Normalize whitespaces in codespans #1085

Merged
merged 7 commits into from
Mar 28, 2024
Merged

Normalize whitespaces in codespans #1085

merged 7 commits into from
Mar 28, 2024

Conversation

gpetiot
Copy link
Collaborator

@gpetiot gpetiot commented Feb 21, 2024

To summarize:

  • A newline followed by any whitespaces should be considered to be one space character by odoc-parser and ocamlformat.
    Consecutive whitespaces not after a newline are conserved as they are.
  • Trim whitespaces at the beginning and end of code spans. Not related to the issue but as we making whitespaces a bit less significant, this is worth mentioning.

@dbuenzli
Copy link
Contributor

Implementing the proposition of https://github.com/tarides/tooling-tasks/issues/49

Please avoid referencing non-public propositions in a public project.

  • A newline followed by any whitespaces should be considered to be one space character by odoc-parser and ocamlformat.

That's quite an odd convention. If you want to change the current behaviour (not sure what it is) I think you should rather ignore all whitespace (and make sure you can introduce one with an escape).

@jonludlam
Copy link
Member

How will this affect mdx?

@dbuenzli
Copy link
Contributor

That's quite an odd convention.

Sorry I misunderstood, I thought it was for code blocks. What you propose is fine with me (and FWIW indirectly what CommonMark does).

@dbuenzli
Copy link
Contributor

  • Trim whitespaces at the beginning and end of code spans. Not related to the issue but as we making whitespaces a bit less significant, this is worth mentioning.

Also I don't think this should be done.

@gpetiot
Copy link
Collaborator Author

gpetiot commented Feb 22, 2024

How will this affect mdx?

There won't be any impact, mdx doesn't process code spans, only the code blocks

@panglesd
Copy link
Collaborator

For the records, here is the issue this PR is solving.

Citing @Julow (private conversation):

[In the context of ocamlformat] Code spans can be large and might need to break. For example, we might want to format the first into the second:, and having the following two versions identical would allow for better formatting:

(** [very_long_word b] *)

(** [very_long_word
     b] *)

In the second case, Odoc's AST contains "very_long_word\n b". When using the html backend, both comments render the same as the whitespaces are normalized by the browser.

My suggestions:

  • A newline followed by any whitespaces should be considered to be one space character by odoc-parser and ocamlformat.
    Consecutive whitespaces not after a newline are conserved as they are.
  • Trim whitespaces at the beginning and end of code spans. Not related to the issue but as we making whitespaces a bit less significant, this is worth mentioning.
  • Backward compatibility is not a concern. Most already written don't have this problem. The newline is not visible when using the html backend.

See also this PR for the discussion on the ocamlformat side.

@dbuenzli
Copy link
Contributor

As I said I don't think initial and trailing whitespace should be suppressed. 1) There may be legitimate reasons to have some 2) It needlessly deviates from other lightweight markup languages like CommonMark.

@panglesd
Copy link
Collaborator

Maybe this PR could be split in two:

  • The modification solving the original issue: considering newlines followed by blanks as a single space, and
  • the initial and final whitespace trimming

so one modification is not blocking the other?

I think having the syntax for inline elements play nice with the "80 char rule" is good to have, so I would be in favor of the first change. I haven't though about the second change, but as a first impression I would be in favor of having the same behavior as CommonMark.

@gpetiot
Copy link
Collaborator Author

gpetiot commented Mar 18, 2024

  • the initial and final whitespace trimming

I removed these changes and cleaned up the PR.

src/parser/lexer.mll Show resolved Hide resolved
Copy link
Collaborator

@panglesd panglesd left a comment

Choose a reason for hiding this comment

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

Maybe a small mention of that in the docs could be good! Odoc's language is underspecified but let's slowly improve that...

src/parser/lexer.mll Show resolved Hide resolved
src/parser/lexer.mll Outdated Show resolved Hide resolved
src/parser/lexer.mll Outdated Show resolved Hide resolved
@panglesd
Copy link
Collaborator

Promoted tests and updated docs, and then it's mergeable!

Copy link
Collaborator

@panglesd panglesd left a comment

Choose a reason for hiding this comment

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

This PR is ready to be merged. I'll let a few days before merging to let anyone who dislike the change speak up.

@jonludlam
Copy link
Member

Looks good to me. In it goes!

@jonludlam jonludlam merged commit efa5024 into ocaml:master Mar 28, 2024
9 of 10 checks passed
@gpetiot gpetiot deleted the normalize-whitespaces-in-codespans branch March 29, 2024 14:51
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.

4 participants