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

Add indented interpolated values feature #11

Open
Martoon-00 opened this issue Apr 8, 2022 · 5 comments
Open

Add indented interpolated values feature #11

Martoon-00 opened this issue Apr 8, 2022 · 5 comments
Labels
feature New functionality that may worth a new release

Comments

@Martoon-00
Copy link
Member

Martoon-00 commented Apr 8, 2022

Clarification and motivation

What would be a replacement for this?:

build ... = mconcat
  [ "Error:"
  , indentF 2 $ build thing1  -- potentially multiline text
  , ""
  , "Extra:"
  , indentF 2 $ build thing2
  ]

Probably we need some special construct for this, this can look like

build ... = [int||
  Error:
    #|{thing1}

  Extra:
    #|s{thing2}
  |]

where #| stands for preserving the current indentation for the multiline text.

Acceptance criteria

  • It is possible to automatically indent the text produced during interpolation.
  • Corner cases, defaults, other use-cases and thought through.
@Martoon-00 Martoon-00 added the feature New functionality that may worth a new release label Apr 8, 2022
@dcastro
Copy link
Member

dcastro commented Apr 10, 2022

Should this behaviour (applying the contextual indentation to all line-wraps) be the default behaviour? And if the user wants line-wraps to have 0-indentation, then use special syntax for that? 🤔

This seems like it's what users will want to happen most often, and nyan-interpolation seems to follow a "do the expected thing by default" philosophy.

@Martoon-00
Copy link
Member Author

That's a very good question.

(1) I agree that this better be default behaviour rather than not. And yeah, use some special syntax for 0-indentation for line-wraps, perhaps some #< then?

putTextLn [int||
  The text:
    #<{multilineText} 
  |]
The text:
  Once upon a time
a hobbit lived
in Shire.

(2) Next question, when will this trigger?
Apparently, if the placeholder has preceding text at the same line, then preserving indentation does not make sense.

So I propose, the indentation of multiline text to trigger only if the placeholder is the first non-whitespace thing at the line - in this case the entire interpolated text will use the respective indentation. Otherwise, nothing special should happen?

putTextLn [int||
  My text:
    #{multilineText}

  And also a text: #{multlineText}, period.
  |]
My text:
  Starts here
  and ends here

And also a text: Starts here
and ends here, period.

I'm not sure that there is a reasonable strategy for the second part of the text here, so probably better just do nothing.

(3) And maybe sometimes it actually makes sense to ignore the triggering rule mentioned above and repeat the text before the placeholder. For instance:

putTextLn [int||
  Expected:
    | #|{expected}
  Got:
    | #|{got}
  |]
Expected:
  | handle do
  |   throwM Exception
Got: 
  | handle do
  |   throwM Exc

(4) Any switches to add? Since we allow fine-tuning via the syntax of the placeholder (# vs #<), I guess no.

But this should be tunable in the interpolator itself indeed. Some people may not like this default behaviour with auto-indentation.

@dcastro
Copy link
Member

dcastro commented Apr 15, 2022

Apparently, if the placeholder has preceding text at the same line, then preserving indentation does not make sense.

Ooh great point!

Ok it seems there's actually multiple possible "indentation / vertical alignment" modes we could implement.
Let's try and list which modes we want to support.

Given this code (the exact placeholder syntax doesn't matter here):

let t = "aa\nbb"

[int||
  Bullet point list:
    - #{t}
|]

We could support the following modes:

  1. 0-indent / no alignment
[int||
  Bullet point list:
    - aa
  bb
|]
  1. Indent at current column. This is similar to the align combinator from wl-pprint-text
[int||
  Bullet point list:
    - aa
      bb
|]
  1. Inherit indentation from the current line.
[int||
  Bullet point list:
    - aa
    bb
|]

wl-pprint-text also has other alignment modes (e.g. hang) but I don't think they're common enough for us to support them all.


And maybe sometimes it actually makes sense to ignore the triggering rule mentioned above and repeat the text before the placeholder

Hmm I'm not sure, I agree it may be useful in some situations, but this seems like a more complex case (seems almost like a "template for a line"). My humble opinion is that this should be outside the scope of nyan-interpolation.


Choosing the default mode:

In my first comment I suggested "Indent at current column" should be the default. But, as you pointed out, it may not always be desirable.

In that case, maybe "Inherit indentation" would be a good default? It would look good in the scenario you described at the very top and in the scenario you described in your point (2).


(4) Any switches to add? Since we allow fine-tuning via the syntax of the placeholder (# vs #<), I guess no.

Yeah I don't think we need switches. Each placeholder should choose how to display itself.

I have no idea what syntax we could for the placeholders to support 3 modes though 😅 Am I making things too complex?
Maybe just supporting "Inherit indentation" and "0-indentation" (and ignoring "Indent at current column") would be enough?

@Martoon-00
Copy link
Member Author

We could support the following modes:

Cool, these all make sense for me 👍

but this seems like a more complex case (seems almost like a "template for a line")

Okay, yeah, I agree here, that's a too far-going proposal.

In that case, maybe "Inherit indentation" would be a good default?

I think that's a very good suggestion.

My proposal from the previous post to have conditional default (indent only if having preceding whitespaces) - on a second thought, it is tricky and unintuitive.

And "inherit indentation" - it is unconditional, but very sensible, takes a perfect balance between "too smart to be default" and "too rarely useful".

Let's really go with it.

I have no idea what syntax we could for the placeholders to support 3 modes though 😅 Am I making things too complex?

Ah no, I personally think this is just fine, and all 3 seem to be worth having 😋

If users find that too complex, they can just not use all 3 😄 Aand, I will likely add a way to define custom "syntax -> indentation behaviour" mappings eventually.

@Martoon-00
Copy link
Member Author

Thanks for this discussion, now everything seems mostly clear, I hope to start implementing this soon 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality that may worth a new release
Projects
None yet
Development

No branches or pull requests

2 participants