open-telemetry / weaver

OTel Weaver lets you easily develop, validate, document, and deploy semantic conventions
Apache License 2.0
55 stars 19 forks source link

Add the ability to word-wrap or limit line length #364

Open lquerel opened 2 months ago

lquerel commented 2 months ago

We should be able to define the maximum line length for comments, for example. Several approaches are possible:

  1. Add a word_wrap filter that would take a maximum number of characters per line as a parameter.
  2. Add an optional parameter to the existing indent filter to specify a maximum number of characters per line, accounting for indentation.
  3. Add a configuration option in the comment_formats section to specify the preferred maximum line length for comments, per language.

These options are not mutually exclusive. Personally, I would be inclined to start with 2 and 3.

MrAlias commented 3 weeks ago

This is needed by the Go SIG for their adoption of weaver.

jsuereth commented 4 days ago

Thinking of using this: https://docs.rs/textwrap/latest/textwrap/

lquerel commented 4 days ago

@jsuereth yes I had the same crate in mind.

jsuereth commented 4 days ago

@lquerel - Looks like it can do a bit of work, but some wonkyness - https://github.com/open-telemetry/weaver/pull/454

jsuereth commented 4 days ago

Alright - the prototype is looking ok, but we'll need to sort out word splits a bit. E.g. The current algorithm in my draft PR does this:

-   // [vendor identifier]: https://developer.apple.com/documentation/uikit/uidevice/1620059-identifierforvendor
+   // [vendor identifier]: https://developer.apple.com/documentation/uikit/
+   // uidevice/1620059-identifierforvendor

I believe I may just update the word-slicing logic to avoid seeing / as a word-break, and see how this is.

The other problematic situation - Markdown and word-wrap interactions:

-   //   - Set `error.type` to capture all errors, regardless of whether they are defined within the domain-specific set or not
+   //   - Set `error.type` to capture all errors, regardless of whether they are
+   // defined within the domain-specific set or not

@MrAlias From a go perspective, if you see markdown lists of this long variety, what would you want to see? Does go format markdown in comments?

I'm going to investigate the library a bit more to see how we could deal with this scenario.

MrAlias commented 4 days ago

The current algorithm in my draft PR does this:

-   // [vendor identifier]: https://developer.apple.com/documentation/uikit/uidevice/1620059-identifierforvendor
+   // [vendor identifier]: https://developer.apple.com/documentation/uikit/
+   // uidevice/1620059-identifierforvendor

I believe I may just update the word-slicing logic to avoid seeing / as a word-break, and see how this is.

+1 to not making this split :+1:

MrAlias commented 4 days ago

The other problematic situation - Markdown and word-wrap interactions:

-   //   - Set `error.type` to capture all errors, regardless of whether they are defined within the domain-specific set or not
+   //   - Set `error.type` to capture all errors, regardless of whether they are
+   // defined within the domain-specific set or not

@MrAlias From a go perspective, if you see markdown lists of this long variety, what would you want to see? Does go format markdown in comments?

Go does format these lists (https://tip.golang.org/doc/comment#lists). Unfortunately, it does not match exactly markdown list formatting rules. It requires a prefix space.

We do utilize this in place (https://pkg.go.dev/go.opentelemetry.io/otel/metric#hdr-Instrument_Name), but for the semconv it is sort-of used just not rendered correctly:

20241108_081036

jsuereth commented 4 days ago

@MrAlias Could you verify this expected test output is my target for rendering Go?

See: https://github.com/open-telemetry/weaver/blob/e54635b8aef2058ca3df207a3e86144fca636e9a/crates/weaver_forge/expected_output/comment_format/example.go And the config: https://github.com/open-telemetry/weaver/blob/e54635b8aef2058ca3df207a3e86144fca636e9a/crates/weaver_forge/templates/comment_format/weaver.yaml#L33

MrAlias commented 4 days ago

Looks great! :rocket:

jsuereth commented 4 days ago

Don't celebrate too early - Think it's going to take a while to get that output :)

Going to have a think about how to use the markdown rendering + word-wrapping library together, it looks like we may be either doing our own thing or very clever point-fixes to word-splits :)