Use soft linebreaks for constant doc styles

[Sija]

Fixes cases like KeyBindingHandler constant entry in Readline docs.

[bew]

Could you show a screenshot before/after to explain visually what this does? Ty

[Sija]

@bew sure!

before:

after:

[oprypin]

I think this is invalid. If code is changed to be the exact same thing as pre, then how do you do code?

[ysbaddaden]

Maybe we should fix the doc templates to use PRE instead for constant summaries, or some kind of inline block? Having a nicely formatted value is definitely nicer, but having CODE == PRE seems too much.

[donovanglover]

From my understanding of it this commit just makes <code></code> sections behave like <pre><code></code></pre> (aka preserve extra spaces and new line characters), but without the extra styling already provided for pre tags (useful in this case).

One important difference, however, is that using white-space: pre will remove text wrapping for all code blocks not wrapped in pre tags, consequentially making the entire page horizontally scrollable given a small enough viewport. This is in contrast to how pre tags have their own scrollbox, since the pre element is display: block and the code element is display: inline.

In any case, I'd suggest using white-space: pre-wrap as opposed to simply pre. If you do, you need to restrict its scope to only the sections you want to change (i.e. .entry-const), so other code blocks that don't need this change aren't changed.

.entry-const code {
    white-space: pre-wrap;
}

Ref:

• https://html.spec.whatwg.org/multipage/grouping-content.html#the-pre-element
• https://developer.mozilla.org/en-US/docs/Web/CSS/white-space

[donovanglover]

In the long term it may be ideal to use pre, but in the short term this pull request fixes the issue at hand. It's a trivial fix for a trivial issue, whereas migrating to pre may require a lot more changes.

I say that we merge this CSS fix for now, until there's a valid reason to change what isn't broken. crystal docs can always be refactored later as Crystal gets closer to 1.0.

[jhass]

Am I missing something or isn't this just that we somewhere generate (from Markdown) a <code> where we should generate a <pre><code>?

[oprypin]

The styling works fully as intended, just that this constant's definition is too scary for docs.

I expect numerous doc regressions if this is merged, because presumably this affects method parameters as well

[Sija]

I've force-pushed changes with @gloverdonovan's suggestion.

[Sija]

Since .entry-const is used in just one place throughout the codebase (src/compiler/crystal/tools/doc/html/type.html) there shouldn't be any regressions. Is it GTG now?

[donovanglover]

Maybe the title should be updated to reflect what this pull request actually does?

[Sija]

@gloverdonovan I updated the PR title accordingly(?)

[Sija]

@ysbaddaden @jhass @oprypin ready for another review

[asterite]

Maybe constant values shouldn't be shown in the docs.

[oprypin]

Values would be fine, it's the chunks of source code that's the problem.

[asterite]

What are values? You mean just show literals like numbers and strings? Maybe that's a good compromise.

[Sija]

@asterite @oprypin do you see any problem with proposed solution? It fixes problem at hand with no downsides. Better options could be discussed in another issue/PR.

[oprypin]

I see it as exacerbating the problem. Before, we have an unwanted chunk of code taking up 3 lines, and after, we have it taking up 8 lines.

[oprypin]

Oh, actually, before this change, the chunk of code is also non-runnable (newlines are missing and not replaced by semicolons). So yeah, it's an improvement, although it kinda hides the root problem.

[asterite]

I think what @oprypin says is exactly the thing. There's a problem: constant values look ugly. So a fix is to make it beautiful. Another fix is to hide them, because they are irrelevant.

I originally included values in constants because of enum members, to know their values (I don't know why, though). So instead of rushing and "fixing" this we are taking some time to discuss what's the real problem and what a good solution might be.

[straight-shoota]

@asterite I strongly disagree on the value of constants being irrelevant for API docs. They are certainly not always meaningful, but on many occassions it's super useful to know. Especially when it's just a plain literal:

MAX_CODEPOINT = 0x10FFFF

It wouldn't hurt to have some lines about the details alongside, but you shouldn't need to wrap that plain value in prose.

If it's more complicated than that, for sure, there is no point in printing all this:

STDERR = (IO::FileDescriptor.new(2, blocking: (LibC.isatty(2)) == 0)).tap do |f| f.flush_on_newline = true end 

Or the Readline example mentioned in the OP.

In such cases, it is essential to have a doc string describing all the details about this constant, it's value and usage. Perhaps it would be nice to add the inferred type alongside the name, but nothing else.

[Sija]

@asterite Before now there was no discussion about constants being irrelevant in docs. This PR fixes only the presentation layer (you have new lines - then show 'em!). ATM we have a mess - we see the values, yet we see them garbled. How bad is fixing this?

[Sija]

From #6118 (comment) by @asterite:

This is an improvement over the old code. It might not fix all problems, but that's how we move in Crystal, fixing things slowly.

Shall we use that approach, merge this and discuss future improvements in another issue/pr?

[asterite]

Actually, what we can do is to change the doc generator to only show simple constants, like numbers, booleans, chars and strings. Everything else could be hidden.

[Sija]

@asterite Personally I'd find it counter-intuitive and misleading. Anyhow, let's discuss this in a new issue/pr please.

[asterite]

@Sija Do you find the value of STDOUT useful?

We can always show a "show source" that leads to where the constant is defined, and there you can see the value.

[oprypin]

Personally I'd find it counter-intuitive and misleading. Anyhow, let's discuss this in a new issue/pr please.

[RX14]

It's impossible (for me) to work out if this is the right change, it's just CSS so lets merge it and see if anything breaks with existing <code> blocks.