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

[wit-parser] Don't trim leading/trailing whitespace from lines in doc comments #1954

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mjoerussell
Copy link

Trimming whitespace in doc comment lines could break custom formatting when reading and/or copying docs from .wit files.

For example, using jco to generate Typescript definitions, we want to copy the documentation directly from .wit to .d.ts. However, a doc comment like this will be altered after parsing:

/// Some documentation:
///     - With indentation
/**
 * Some documentation
 * - With indentation
 */

@@ -4,4 +4,4 @@ Caused by:
0: failed to decode world from module
1: module was not valid
2: failed to resolve import `a::a`
3: module requires an import interface named `a`
3: module requires an import interface named `a`
Copy link
Member

Choose a reason for hiding this comment

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

Could you try rebasing on main and re-BLESS-ing tests to remove some of these changes?

@alexcrichton
Copy link
Member

Thanks for this! One thing I might recommend is to flesh out this trimming of whitespace more than what's already there. You're right that the current implementation is buggy but ideally this wouldn't pick up the whitespace from the comment itself (e.g. the leading space). The way I know I've dealt with this in the past with rustdoc is to collect the documentation block, calculating the longest longest amount of whitespace shared amongst all lines, and then drop that. That would, for example, drop the leading space here.

@mjoerussell
Copy link
Author

The way I know I've dealt with this in the past with rustdoc is to collect the documentation block, calculating the longest longest amount of whitespace shared amongst all lines, and then drop that. That would, for example, drop the leading space here.

Sounds good, I wasn't sure if this was an issue for the project in the first place or what behavior would be a good replacement so I wanted to keep the changes minimal to start.

@mjoerussell mjoerussell force-pushed the wit-parser-docs-no-trim branch from 0e4b337 to bd39d87 Compare December 17, 2024 19:23
@alexcrichton
Copy link
Member

Sounds good, and yeah no worries! I think some git mishaps may have happened during the rebase though?

…e minimum amount of whitespace common to all lines and strip that much, rather than going all-or-nothing.
@mjoerussell mjoerussell force-pushed the wit-parser-docs-no-trim branch from bd39d87 to 7507a92 Compare December 17, 2024 22:58
@mjoerussell
Copy link
Author

I think some git mishaps may have happened during the rebase though?

Yeah sorry about that, I think I rebased and pushed using --force-with-lease by mistake. Should be fixed now.

* Rename intermediate value `d` to `contents`
@alexcrichton
Copy link
Member

I think there might need to be another BLESS=1 run, and maybe a test or two failing as well?

@mjoerussell
Copy link
Author

@alexcrichton Sorry for the delay, got sidetracked with other work. I've been seeing one test, wit-with-all-features.wit.stdout, consistently fail without BLESSing. When I use BLESS=1 the test passes, as expected, but no new output is written to the expectation file so the next run will fail again.

The problematic diff is a trailing newline at the end of the file - the output file is written without it, but then the runtime test contents include it. I noticed that I could fix this in tests/cli.rs, without causing any other tests to fail, by not popping the trailing newline from the expectation file in assert_output().

It's a small change, but I'm not familiar enough with the project to say that it won't cause other issues. I'd love to hear what you think about this, and if you want to go in a different direction I'll keep looking into alternatives.

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.

2 participants