Skip to content

Conversation

@CreeptoGengar
Copy link
Contributor

Fix incorrect indentation handling in documentation comment extraction.
The previous implementation only removed a single space after comment markers, which caused issues with multi-line comments having varying indentation levels.
Now the code finds the minimum common indentation across all lines in a comment block and removes it uniformly, preserving
relative alignment within the block.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @CreeptoGengar)


a discussion (no related file):
would suggest adding a test to show the improvement.

@CreeptoGengar
Copy link
Contributor Author

Added a test case for the indentation fix.
It covers multi-line doc comments with varying indentation levels and verifies that the minimum common indentation is correctly removed while preserving relative alignment between lines.

@CreeptoGengar CreeptoGengar requested a review from orizi December 7, 2025 11:01
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

@orizi reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @CreeptoGengar and @FroyaTheHen)


a discussion (no related file):
@FroyaTheHen

Copy link
Collaborator

@FroyaTheHen FroyaTheHen left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @CreeptoGengar and @orizi)


crates/cairo-lang-doc/src/tests/test-data/indentation.txt line 1 at r2 (raw file):

//! > Documentation

Can you please add a test case that would have failed before but now passes?


crates/cairo-lang-doc/src/tests/test-data/indentation.txt line 43 at r2 (raw file):

This line has extra indentation.
This line has less indentation.
Another indented line.

I understand that the behaviour expected from those changes would be to preserve the indentation in a way that would remove the number of leading whitespaces common to all the comment lines. (Please argue otherwise if you do not agree.) Although the test shows all of the leading whitespaces are removed. Isn't the expected 👇 ??

Suggestion:

//! > Item documentation #2
Function with multi-line doc comment.
    This line has extra indentation.
  This line has less indentation.
    Another indented line.

Updated indentation in documentation comments to ensure consistent formatting and clarity.
Updated the extract_comment_line_content function to preserve indentation after the comment marker in the extracted content.
Added logic to restore indentation for documentation comments by mapping line indices to their leading spaces. Enhanced text processing to handle both exact and partial line matches for improved formatting.
Copy link
Collaborator

@FroyaTheHen FroyaTheHen left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @CreeptoGengar and @orizi)


crates/cairo-lang-doc/src/tests/test-data/indentation.txt line 43 at r2 (raw file):

Previously, FroyaTheHen (Kinga Cepielik) wrote…

I understand that the behaviour expected from those changes would be to preserve the indentation in a way that would remove the number of leading whitespaces common to all the comment lines. (Please argue otherwise if you do not agree.) Although the test shows all of the leading whitespaces are removed. Isn't the expected 👇 ??

@CreeptoGengar, I'm afraid I cannot accept the changes. For couple of reasons:

  1. This is going to break scarb doc formatting. As scarb doc uses this code in documentation generation. If the indents were preserved if only a 4spaces or a tab is used - markdown evaluates it to code-block which would be misleading for the documentation users.
  2. Similarly this would affect the ls.
  3. The feature brings little value yet complicates the parsing logic significantly (the current implementation would not be enough as it only addresses a case where the doc comment doesn't combine with other markdown markers - run cargo test -p cairo-lang-doc to see).

Sorry, I could have checked that earlier on. To give you a bit more context, the comment: TODO(mkaput): The way how removing this indentation is performed is probably ... is outdated as it existed before the significant changes (mdbook-parsing-style entirely took over) were made in crates/cairo-lang-doc/src/parser.rs file.

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.

4 participants