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 raw text-block indentation #4655

Merged
merged 12 commits into from
Jan 31, 2024
Merged

Fix raw text-block indentation #4655

merged 12 commits into from
Jan 31, 2024

Conversation

ChrisPenner
Copy link
Contributor

@ChrisPenner ChrisPenner commented Jan 30, 2024

Overview

Current parse/print of ''' text literals ends up adding a layer of indentation to the literal on every round-trip.

Solutions considered:

  1. (Implemented): When parsing a ''' literal, dedent the literal by the indentation of the opening ''', UNLESS there's non-whitespace characters in the area we're de-denting, in which case leave the literal as-is.
  • Pros: All possible text-literals are representable
  • Cons: Text is parsed differently depending on its contents.
  1. Dedent the entire text literal by the largest common indent of the literal.
  • Pros: Is a simple, consistent rule that's mostly intuitive.
  • Cons: This makes some text-literals unrepresentable.

Implementation notes

I implemented option 1, which involved:

  • Trim whitespace equal to the column number of the opening delimiter, if possible on every line, otherwise don't trim anything.
  • Adds a newline within {{ }} delimiters for multi-line blocks, without it they would do strange things to the indentation of code-fence blocks; plus it looks nicer IMO

Interesting/controversial decisions

It may be a bit unintuitive that the behaviour changes depending on the actual text in the literal, but chatted with @runarorama and we think this is the simplest way to go that accomplishes the primary goal of roundtrips working.

Test coverage

Transcripts

@ChrisPenner ChrisPenner self-assigned this Jan 31, 2024
@ChrisPenner ChrisPenner force-pushed the cp/text-codeblock-fix branch 2 times, most recently from 1b90144 to 51b1351 Compare January 31, 2024 00:28
@ChrisPenner ChrisPenner force-pushed the cp/text-codeblock-fix branch from 51b1351 to e66806f Compare January 31, 2024 00:30
@ChrisPenner ChrisPenner mentioned this pull request Jan 31, 2024
5 tasks
@pchiusano
Copy link
Member

pchiusano commented Jan 31, 2024

IIRC the current implementation is that:

  1. Newline after the opening quote, and just before closing quote is ignored. This avoids awkwardness of needing to put the closing quote on the same line as the end of the string (I’d like to keep this). Also avoids needing to put the content on the same line as the opening quote (also important). Also in conjunction with 2, it allows the opening quote to hang after an =, with the content indented below (this PR will break this behavior but that seems ok to me).
  2. Indentation is stripped up to the first non-whitespace character on the first line.

If you’re doing ascii art where the first line starts in the middle of the line, then this heuristic of 2 doesn’t work and this is where round trip errors are happening. Other blocks work fine I think.

@@ -304,10 +306,12 @@ fix_4384b = {{ {{ docExampleBlock 0 '99 }} }}
fix_4384c : Doc2
fix_4384c =
use Nat +
{{ {{ docExampleBlock 0 do
{{
{{ docExampleBlock 0 do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This block is a smidge strange, but it does round-trip. Rúnar and I tried to improve it without much luck :'(

@ChrisPenner ChrisPenner force-pushed the cp/text-codeblock-fix branch from 76efa3b to 4752b44 Compare January 31, 2024 20:41
@ChrisPenner ChrisPenner marked this pull request as ready for review January 31, 2024 21:05
@ChrisPenner ChrisPenner requested a review from aryairani January 31, 2024 21:05
@aryairani aryairani merged commit 9e4bc32 into trunk Jan 31, 2024
7 checks passed
@aryairani aryairani deleted the cp/text-codeblock-fix branch January 31, 2024 23:02
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.

3 participants