Skip to content

Index multiline comments#632

Open
soutaro wants to merge 3 commits intomainfrom
comment-lines
Open

Index multiline comments#632
soutaro wants to merge 3 commits intomainfrom
comment-lines

Conversation

@soutaro
Copy link
Contributor

@soutaro soutaro commented Mar 4, 2026

#87

An RBS CommentNode object represents sequence of comment lines, but the Rubydex Comment object represents a line of comment. This PR implements the conversion from one CommentNode to list of Comment objects.

@soutaro soutaro requested a review from a team as a code owner March 4, 2026 07:57
@soutaro soutaro added the bugfix A change that fixes an existing bug label Mar 4, 2026
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

It would be nice to run a quick benchmark against rbs core and stdlib to understand if there's significant performance impact of trying to split the comments.

}

#[test]
fn split_multiline_comments() {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a test with an escaped line break as part of the comment's content. It's easy for this to get confused.

# splits strings at the \\n char
def foo; end


#[test]
fn split_multiline_comments() {
let source = "# First line\n# Second line\n# Third line\nclass Foo\nend";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use index_source and nicely indent the example like we do in other tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also have a test that shows how it behaves with an initial indent?

module Outer
  # First line
  # Second line
  class Inner
  end
end

@soutaro
Copy link
Contributor Author

soutaro commented Mar 5, 2026

I run a quick benchmark with RBS files in ruby/rbs repo. It's slightly slower, but I don't think it's problematic. (Especially for importing core definitions.)

Details

Before

➜  rust git:(main) target/release/rubydex_cli --stats ../../rbs/core ../../rbs/stdlib ../../rbs/sig

Query statistics
  Total declarations:         3810
  With documentation:         2030 (53.3%)
  Without documentation:      1780 (46.7%)
  Total documentation size:   2034 bytes
  Multi-definition names:     66 (1.7%)

Declaration breakdown:
  Constant               2513
  Class                  1060
  Module                  186
  GlobalVariable           51

Definition breakdown:
  Type                    Total   Linked   Orphan
  ----                    -----   ------   ------
  Constant                 2513     2513        0
  Class                    1134     1134        0
  Module                    376      376        0
  GlobalVariable             51       51        0
  ----                    -----   ------   ------
  TOTAL                    4074     4074        0
  Orphan rate: 0.0%

Timing breakdown
  Listing             0.002s ( 10.8%)
  Indexing            0.010s ( 51.2%)
  Resolution          0.007s ( 33.7%)
  Querying            0.001s (  4.4%)
  Cleanup             0.000s (  0.0%)
  Total:              0.020s

Maximum RSS: 20480000 bytes (19.53 MB)

Indexed 330 files
Found 3810 names
Found 4074 definitions
Found 330 URIs

After

➜  rust git:(comment-lines) ✗ target/release/rubydex_cli --stats ../../rbs/core ../../rbs/stdlib ../../rbs/sig

Query statistics
  Total declarations:         3810
  With documentation:         2030 (53.3%)
  Without documentation:      1780 (46.7%)
  Total documentation size:   1015095 bytes
  Multi-definition names:     66 (1.7%)

Declaration breakdown:
  Constant               2513
  Class                  1060
  Module                  186
  GlobalVariable           51

Definition breakdown:
  Type                    Total   Linked   Orphan
  ----                    -----   ------   ------
  Constant                 2513     2513        0
  Class                    1134     1134        0
  Module                    376      376        0
  GlobalVariable             51       51        0
  ----                    -----   ------   ------
  TOTAL                    4074     4074        0
  Orphan rate: 0.0%

Timing breakdown
  Listing             0.005s ( 21.5%)
  Indexing            0.013s ( 50.6%)
  Resolution          0.006s ( 24.7%)
  Querying            0.001s (  3.1%)
  Cleanup             0.000s (  0.0%)
  Total:              0.025s

Maximum RSS: 19578880 bytes (18.67 MB)

Indexed 330 files
Found 3810 names
Found 4074 definitions
Found 330 URIs

if has_docs {
declarations_with_docs += 1;
let doc_size: usize = definitions.iter().map(|def| def.comments().len()).sum();
let doc_size: usize = definitions.iter().map(|def| def.comments().iter().map(|c| c.string().len()).sum::<usize>()).sum();
Copy link
Contributor Author

@soutaro soutaro Mar 5, 2026

Choose a reason for hiding this comment

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

It says bytes, but it actually was lines. (lines would also make sense, but this is a fix to make it bytes.)

Comment on lines +176 to +180
let line_text = if i == 0 {
line.to_string()
} else {
line[indent_len..].to_string()
};
Copy link
Member

Choose a reason for hiding this comment

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

Is this assuming that all comment lines are indented the same? What happens in this case? Let's add a test to document the behaviour.

class Foo
  # my first comment starts out great
# now I mess up the indent
  def bar: () -> void

# this one is the opposite
  # I fix it later
  def baz: () -> void
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.

My bad. 🙇‍♂️
I was thinking the RBS parser rejects the differently indented comments, but it actually accepts. I need to fix the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix A change that fixes an existing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants