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 the ability to word-wrap or limit line length #364

Open
lquerel opened this issue Sep 12, 2024 · 10 comments · May be fixed by #454
Open

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

lquerel opened this issue Sep 12, 2024 · 10 comments · May be fixed by #454
Assignees
Labels
enhancement New feature or request

Comments

@lquerel
Copy link
Contributor

lquerel commented Sep 12, 2024

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
Copy link

MrAlias commented Oct 18, 2024

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

@jsuereth
Copy link
Contributor

jsuereth commented Nov 8, 2024

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

@lquerel
Copy link
Contributor Author

lquerel commented Nov 8, 2024

@jsuereth yes I had the same crate in mind.

@jsuereth
Copy link
Contributor

jsuereth commented Nov 8, 2024

@lquerel - Looks like it can do a bit of work, but some wonkyness - #454

@jsuereth
Copy link
Contributor

jsuereth commented Nov 8, 2024

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
Copy link

MrAlias commented Nov 8, 2024

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 👍

@MrAlias
Copy link

MrAlias commented Nov 8, 2024

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
Copy link
Contributor

jsuereth commented Nov 8, 2024

@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:

@MrAlias
Copy link

MrAlias commented Nov 8, 2024

Looks great! 🚀

@jsuereth jsuereth linked a pull request Nov 8, 2024 that will close this issue
3 tasks
@jsuereth
Copy link
Contributor

jsuereth commented Nov 8, 2024

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Next Release
Development

Successfully merging a pull request may close this issue.

3 participants