Skip to content

Preserve original indentation in generated documentation #414

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jun 4, 2025
Merged

Preserve original indentation in generated documentation #414

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6772f51
Preserve original indentation in generated documentation
aj-foster May 23, 2025
647c726
Improve efficiency of indentation normalization
aj-foster May 31, 2025
315bff8
Add indented comment to existing documentation test
aj-foster May 31, 2025
14bdbed
Use single regex as conditional for indentation calculation
aj-foster Jun 4, 2025
5257a22
Update lib/protobuf/protoc/generator/comment.ex
whatyouhide Jun 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion lib/protobuf/protoc/generator/comment.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,31 @@ defmodule Protobuf.Protoc.Generator.Comment do
defp format_comment(location) do
[location.leading_comments, location.trailing_comments | location.leading_detached_comments]
|> Enum.reject(&is_nil/1)
|> Enum.map(&String.replace(&1, ~r/^\s*\*/, "", global: true))
|> Enum.join("\n\n")
|> String.replace(~r/\n{3,}/, "\n")
|> normalize_indentation()
|> String.trim()
end

defp normalize_indentation(comments) do
indentation =
String.split(comments, "\n")
|> Enum.reject(&String.match?(&1, ~r/^\s*$/))
|> Enum.map(fn line ->
Regex.run(~r/^\s*/, line, capture: :first)
|> List.first()
|> String.length()
end)
|> Enum.min(fn -> 0 end)
Copy link
Collaborator

Choose a reason for hiding this comment

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

To do a single pass on the list, use Enum.min_by/3 instead of these three passes on the list.

Suggested change
|> Enum.reject(&String.match?(&1, ~r/^\s*$/))
|> Enum.map(fn line ->
Regex.run(~r/^\s*/, line, capture: :first)
|> List.first()
|> String.length()
end)
|> Enum.min(fn -> 0 end)
Enum.min_by(split_comments, fn line ->
if line =~ ~r/^\s*$/ do
0
else
~r/^\s*/
|> Regex.run(line, capture: :first)
|> List.first()
|> String.length()
end
end)

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 had to work on this a bit more. 647c726 replaces this phrase with an Enum.reduce because we need the following properties:

  1. Empty (or whitespace-only) lines should not affect the minimum
  2. We want to end up with a number, rather than the original line.

The reduce leans on the global sort order (:unset is greater than any integer) which may not be the most clear. Another option would be to use a tagged accumulator. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tried turning this into a single regex expression:

    case Regex.run(~r/^(\s+)(?=\S)/, line, capture: :first) do
      [first] -> String.length(first)
      _ -> 0
    end

Should be equivalent and a bit faster

Copy link
Contributor Author

@aj-foster aj-foster May 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@v0idpwn is that based on the suggestion above, or the changes in 647c726? As mentioned above, we don't want empty (or whitespace-only) lines to force a minimum of 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aj-foster the comment from @v0idpwn still applies, you can use a single regex run instead of the if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this change in 14bdbed. 👍🏼


comments
|> String.split("\n")
|> Enum.map(fn line ->
String.replace_prefix(line, String.duplicate(" ", indentation), "")
end)
|> Enum.join("\n")
end

@doc """
Finds a comment via the context. Returns an empty string if the
comment is not found or if `include_docs?` is set to false.
Expand Down
9 changes: 1 addition & 8 deletions lib/protobuf/protoc/generator/util.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,7 @@ defmodule Protobuf.Protoc.Generator.Util do
@spec pad_comment(String.t(), non_neg_integer()) :: String.t()
def pad_comment(comment, size) do
padding = String.duplicate(" ", size)

comment
|> String.split("\n")
|> Enum.map(fn line ->
trimmed = String.trim_leading(line, " ")
padding <> trimmed
end)
|> Enum.join("\n")
String.replace(comment, "\n", "\n" <> padding)
end

@spec version() :: String.t()
Expand Down