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

Provide max line-length in comment filter. #454

Merged
merged 23 commits into from
Nov 24, 2024

Conversation

jsuereth
Copy link
Contributor

@jsuereth jsuereth commented Nov 8, 2024

Provides naive word-wrapping capabilities to the comment filter.

Fixes #364

  • Adds new word_wrap config section with basic set of features for controlling word-wrapping
  • Pulls in textwrap library, but only for pieces. Unfortunately, we need dynamic word-wrapping that is syntax-aware of markdown and html.
  • Adds new configuration parameter for rendering ordered lists in golang's desired format.
  • Adds a configuration parameter to determine if you ignore endlines when word wrapping to repack paragraphs.
    Note: this feature is naive and likely needs to be reworked, longer term
  • Adds a new set of tests to ensure word-wrapping works

@jsuereth
Copy link
Contributor Author

Frustratingly, I'm running into an oddity in go fmt that I'm not sure we should make the default for weaver or somehow configure.

Specifically the go fmt comment spec states the following about lists:

Gofmt reformats bullet lists to use a dash as the bullet marker, two spaces of indentation before the dash, and four spaces of indentation for continuation lines.

Gofmt reformats numbered lists to use a single space before the number, a period after the number, and again four spaces of indentation for continuation lines.

I'm not positive why there's a single-space before number lists and two spaces before dash lists, but we do NOT do this in weaver today.

@lquerel Thoughts? Should I create a specific renderer for markdown for Go that abides by these restrictions?

Additionally, go fmt does not allow nested lists.

@MrAlias What should we do if we have a nested list? Will Go (and go fmt) ignore this or fail?

@jsuereth jsuereth marked this pull request as ready for review November 23, 2024 15:43
@jsuereth jsuereth requested a review from a team as a code owner November 23, 2024 15:43
Copy link

codecov bot commented Nov 23, 2024

Codecov Report

Attention: Patch coverage is 83.26996% with 44 lines in your changes missing coverage. Please review.

Project coverage is 73.9%. Comparing base (1529f30) to head (d6cd29d).

Files with missing lines Patch % Lines
crates/weaver_forge/src/formats/html.rs 68.3% 19 Missing ⚠️
crates/weaver_forge/src/formats/markdown.rs 87.2% 13 Missing ⚠️
crates/weaver_forge/src/extensions/code.rs 55.5% 8 Missing ⚠️
crates/weaver_forge/src/error.rs 0.0% 2 Missing ⚠️
crates/weaver_forge/src/formats/mod.rs 97.5% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #454     +/-   ##
=======================================
+ Coverage   73.1%   73.9%   +0.7%     
=======================================
  Files         49      50      +1     
  Lines       3716    3901    +185     
=======================================
+ Hits        2718    2883    +165     
- Misses       998    1018     +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

* <li>Use a domain-specific
* attribute
* <li>Set {@code error.type}
* to capture all errors,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is indeed strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to more formally pull words and remember prefix/postfix space. The library allows this and uses it, it just doesn't understand changing the line prefix over time.

I hope we can, eventually, fix this all up, I just ran out of time to tinker

Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

Some minor suggestions to clarify the code in certain places. The number of TODOs is not reassuring, but the test results are reassuring.
Thank you!

crates/weaver_forge/src/formats/markdown.rs Outdated Show resolved Hide resolved
if !self.markdown.ends_with("\n\n") && !self.markdown.is_empty() {
self.markdown.push('\n');
let _ = self
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not using the add_blank_line defined just after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It adds a prefix, this does not. add_blank_line should be fine, I may need to go update the prefix handling code.

if let Some(buf) = self.unbreakable_buffer.as_ref() {
// ToDo - we should error out here.
// For now, we just FLUSH this to write to the buffer.
let _ = self
Copy link
Contributor

Choose a reason for hiding this comment

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

add_blank_line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question - when this happens our algorithm is basically broken. I debated between "panic"-ing and implementing.

Regarding blank lines, we have no idea what we've been buffering or why we need to buffer twice, so I think it's unlikely we have a necessary blank line, it's more likely we somehow have nested code + strong/emphasis blocks.

if let Some(buf) = self.unbreakable_buffer.as_mut() {
buf.push_str(text);
} else {
let _ = self
Copy link
Contributor

Choose a reason for hiding this comment

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

add_blank_line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, unbreakable text == a cohesive word that cannot be split, e.g.

[name]: link <- we treat this as unbreakable text.

crates/weaver_forge/src/formats/markdown.rs Outdated Show resolved Hide resolved
@jsuereth jsuereth merged commit aeae196 into open-telemetry:main Nov 24, 2024
24 checks passed
@jsuereth jsuereth deleted the fix-364 branch November 24, 2024 14:33
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.

Add the ability to word-wrap or limit line length
2 participants