Show line numbers at link when there are filename duplicates in "Defined in:" section (docs) #6280

wooster0 · 2018-06-27T12:54:08Z

For example this Defined in: section example:

Defined in:

Would change to this:

Defined in:

colorize.cr:79
colorize.cr:126
foo.cr
bar.cr

for more readability.

And this PR also fixes identical link generations like in the Defined in: section of Float32 where the two primitives.cr links lead to the same line and file because of macros.

asterite

You should prevent duplicates when recording the locations. That's in types.cr

wooster0 · 2018-06-27T13:17:20Z

I couldn't find any types.cr file which has to do with the docs so I did it in generator.cr now.

asterite · 2018-06-27T13:20:26Z

Right, it doesn't have to do with the docs. It has to do with when the compiler tracks these locations. Here: https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/types.cr#L656-L659 . You can probably do locations << location unless locations.include?(location). I say that because another solution could be to use a Set, but that's too heavyweight, and chances of defining a same type in a same file (or even in multiple files) are very low.

jhass · 2018-06-27T13:26:28Z

A location includes a line number, we just don't show it in the docs, right? So I'm actually not sure we should deduplicate based on the filename globally.

wooster0 · 2018-06-27T13:30:16Z

@jhass Hmm you are right. So it would be maybe better to show the Defined in: section like this:

Defined in:

colorize.cr (Line 79)
colorize.cr (Line 126)

WDYT?

asterite · 2018-06-27T13:36:35Z

Well, the link already leads you to the line number. Maybe showing the line number will be just noise, I don't know. A smarter thing to do might be to just show the line number of there are duplicate files. In any case, the current way (before this PR) is acceptable, I think.

This prevents 2 exactly same links like on https://crystal-lang.org/api/0.25.0/Float32.html

wooster0 · 2018-06-27T15:27:34Z

Okay now the Defined in: section of for example Colorize would look like this:

Defined in:

colorize.cr
colorize.cr (line 126)

So it only shows the line number on filename duplicates. IMO showing the line number at every link would be too much.

And in the last commit I fixed that exactly the same links are generated like in the Defined in: section on https://crystal-lang.org/api/0.25.0/Float32.html. The primitives.cr links point to the same line number.

Sija · 2018-06-27T16:03:07Z

src/compiler/crystal/tools/doc/generator.cr

@@ -360,12 +360,13 @@ class Crystal::Doc::Generator
    filename[@base_dir.size..-1]
  end

-  record RelativeLocation, filename : String, line_number : Int32, url : String? do
+  record RelativeLocation, filename : String, line_number : Int32, url : String?, duplicate : Bool do


This logic should IMO be moved to the template.

What do you mean by template? Should I check if it's a duplicate at the type.html ECR file?

Yep, that's what I meant.

Please don't. Templates should be mostly logic-less. This way is fine.

It's not a duplicate though (still different line numbers).

Should I rename it to filename_duplicate then? I wanted to keep the name simple and didn't tried to make the name very accurate.

Don't use simple names. Use expressive names. As short as possible, though. I'd suggest something like multiple_locations_in_file.

straight-shoota · 2018-06-27T19:08:43Z

src/compiler/crystal/tools/doc/generator.cr

    def to_json(builder : JSON::Builder)
      builder.object do
        builder.field "filename", filename
        builder.field "line_number", line_number
        builder.field "url", url
+        builder.field "duplicate", duplicate


I don't think this field needs to be serialized. It doesn't have much meaning outside the immediate template context.
If need be, this can be easily determined by a consuming application.

But don't I need to do this so I can use it in the type.html ECR file?

No. ECR templates directly access the instance methods of the Crystal object. This to_json method is only for generating a JSON export.

straight-shoota · 2018-06-27T19:11:23Z

src/compiler/crystal/types.cr

@@ -655,7 +655,7 @@ module Crystal
    # Adds a location to this type.
    def add_location(location : Location)
      locations = @locations ||= [] of Location
-      locations << location
+      locations << location unless locations.any? { |loc| loc.filename == location.filename && loc.line_number == location.line_number }


What's the reason for adding this condition? Is there a likely case for a type to be defined multiple times in the same line?

This should prevent that generations like in https://crystal-lang.org/api/0.25.0/Float32.html happen where 2 exactly same links are generated.

Ah, it's because of macro expansion. Yeah, then it's probably better to leave this in.
I'd add a comment about this, though.

straight-shoota · 2018-06-27T19:12:19Z

src/compiler/crystal/tools/doc/generator.cr

@@ -386,7 +387,9 @@ class Crystal::Doc::Generator
      filename = filename[1..-1] if filename.starts_with? File::SEPARATOR
      filename = filename[4..-1] if filename.starts_with? SRC_SEP

-      locations << RelativeLocation.new(filename, location.line_number, url)
+      duplicate = locations.any? { |location| location.filename == filename }


The duplicate flag should also be set on the other location.

straight-shoota · 2018-06-27T19:17:40Z

I like the idea of showing the line numbers. But I'd suggest the format colorize.cr:76 which is typically used to specify a file + line location and less noisy than the current suggestion colorize.cr (line 76).

With this shortened format it might even be an option to always display the line number. But I'm not sure about that. Generally, it's not very useful information.
But it would be nice to add it in the title attribute of each link.

wooster0 · 2018-06-27T19:41:24Z

I agree. colorize.cr:76 is a much better format.

Heres how the format on all links would look at for example Debug:

Defined in:

debug/elf.cr:1
debug/dwarf/abbrev.cr:3
debug/dwarf/info.cr:4
debug/dwarf/line_numbers.cr:1
debug/dwarf/strings.cr:1
debug/dwarf.cr:6

And heres how it would look when only showing it at filename duplicates:

Defined in:

colorize.cr
colorize.cr:126

I vote for the first one.
WDYT?

straight-shoota · 2018-06-27T19:49:04Z

I think it's unnecessary to show the line numbers when there is only one location per file. But when there are multiple locations in the same file, all should show the line number.

Defined in:

foo.cr
colorize.cr:79
colorize.cr:126

wooster0 · 2018-06-27T20:44:06Z

I'd rather show the line number at all links. Would be more uniform and only showing it on links with multiple locations more seems confusing to me.

straight-shoota · 2018-06-27T20:55:42Z

The vast majority of types doesn't have multiple definitions in the same file. This is actually pretty rare. Let's not bother to much about uniformity in a few places when we can reduce noise everywhere else.

wooster0 · 2018-06-28T07:35:33Z

The Travis CI fail looks like a runtime error to me. But I don't know why the test_darwin CI failed and why this happens:

  1) YAML::Builder writes scalar
     Failure/Error: string.should eq(expected)

       Expected: "--- 1\n...\n"
            got: "--- 1\n"

     # spec/std/yaml/builder_spec.cr:8

Maybe because of the unless that I added at this line?

straight-shoota · 2018-06-28T10:29:04Z

This CI failure seems to be completely unrelated. This PR doesn't change anything close to YAML builder. And linux specs pass.

It looks like maybe some libyaml issue.

You could try to trigger a CI re-run to ensure it's not a one-time thing.

wooster0 · 2018-06-28T11:07:40Z

Thats weird. test_darwin CI failed again with the same errors.


Failures:

  1) YAML::Builder writes scalar
     Failure/Error: string.should eq(expected)

       Expected: "--- 1\n...\n"
            got: "--- 1\n"

     # spec/std/yaml/builder_spec.cr:8

I'll comment out the unless and see if it still fails.

Edit: It still fails.

straight-shoota · 2018-06-28T12:18:20Z

This is related to the homebrew formula for libyaml being updated to 0.2.1. Don't worry about it in this PR. See #6283

RX14 · 2018-06-29T18:13:33Z

I'm fine with the current behaviour. I don't think there's a need to deduplicate these because they point to different line numbers.

wooster0 · 2018-06-29T18:46:45Z

@RX14 Yeah that was a bad idea. I forgot to change the title and description of the PR. I changed it now.

Sija · 2018-06-29T18:55:00Z

src/compiler/crystal/tools/doc/generator.cr

@@ -360,12 +360,15 @@ class Crystal::Doc::Generator
    filename[@base_dir.size..-1]
  end

-  record RelativeLocation, filename : String, line_number : Int32, url : String? do
+  record RelativeLocation, filename : String, line_number : Int32, url : String?, show_line_number : Bool do


show_line_number -> show_line_number?

I'm not sure if this is a better name for show_line_number. You normally use this ? at the end of the name of a Bool method where you are checking something (like uppercase?).
Or do you mean show_line_number : Bool??

Well, it is a Bool returning method, quite query-ish (show_...) for my taste.

Could we reduce the level of bikeshedding in comments? ❤️

@show_line_number? is not even a valid name for an instance variable.

Sorry :/ @straight-shoota No, but it's still a valid name for an accessor.

Sorry, but show_line_number is perfectly clear and fine. There's no improvement at all to use ? at the end here, it's not like this is an API that's used all over the place, and adding that ? will mean defining the getter in a block, which will make the code less clear.

No, but it's still a valid name for an accessor.

You can't use it in the record definition. Which means you'd need to declare an additional getter in the record body. Not worth it. It's just a simple data struct.

RX14 · 2018-06-30T11:52:14Z

src/compiler/crystal/tools/doc/generator.cr

-      locations << RelativeLocation.new(filename, location.line_number, url)
+      show_line_number = locations.any? do |location|
+        if location.filename == filename
+          location.show_line_number = true


How does this even work? location is a struct.

Thats because of the setter I added for show_line_number in line 364.

But it's a struct. When you do locations.any? do |location|, that location is a copy of the struct saved in the array, so it shouldn't be modified. Are you sure this is working? Did you compile the compiler and ran the docs?

(we need specs for the docs generator)

Ok fixed it now by making RelativeLocation a class. It's working. Tested it. And at type.html it needs to be like that on one line otherwise it would be shown like colorize.cr :126.

I'm not sure if this is a good idea. Making it a class adds lots of heap allocations. Why not just change the code to work with a struct? locations is an array and instead of using any? you can just loop through it with each_with_index and store the updated entry back into the array.

RX14 · 2018-07-16T09:33:13Z

src/compiler/crystal/tools/doc/generator.cr

@@ -360,7 +360,13 @@ class Crystal::Doc::Generator
    filename[@base_dir.size..-1]
  end

-  record RelativeLocation, filename : String, line_number : Int32, url : String? do
+  class RelativeLocation
+    setter show_line_number


Make this a property and remove it from the getter macro please

Ah yes that's a good idea

straight-shoota

The change from struct to class is unnecessary, this should work with record (see https://github.com/crystal-lang/crystal/pull/6280/files#r202628750).

RX14 · 2018-07-16T21:48:55Z

@straight-shoota please lets not prematurely optimize and make the code far less readable. Lots of allocations happen in the compiler. This being a class is just fine.

straight-shoota · 2018-07-16T22:01:37Z

I'd say it's just throwing away (a little bit of) performance for no real reason.

It's just

show_line_number = locations.any? do |location|
  if location.filename == filename
    location.show_line_number = true
  end
end

vs.

show_line_number = false
locations.each_with_index do |location, i|
  if location.filename == filename
    locations[i] = location.copy_with(show_line_number: true)
    show_line_number = true
    break
  end
end

Yes, No. 1 is a bit shorter, but it's hardly less readable.

wooster0 · 2018-07-24T18:13:38Z

Hmm ok I rather changed it to a struct now. Let's try to prevent as much heap allocations as possible. It's actually still pretty readable I think.

Edit: Hmm I tested it now and unfornately it doesn't seems to work actually.. I'm not sure why but for some reason the Defined in: section is just empty at me.
Let's please just keep it simple and use classes.

RX14 · 2018-07-28T16:44:17Z

Another review from a core team member please, this is ready to go

sdogruyol

Thank you @r00ster91 👍

Prevent link duplicates in Defined in: section

5961965

asterite reviewed Jun 27, 2018

View reviewed changes

Prevent duplicates in generator.cr

295841e

Show line number when it's a duplicate

7c6a73f

jhass approved these changes Jun 27, 2018

View reviewed changes

Prevent duplicate links who point to the same line number and filename

b52654b

This prevents 2 exactly same links like on https://crystal-lang.org/api/0.25.0/Float32.html

Sija reviewed Jun 27, 2018

View reviewed changes

straight-shoota requested changes Jun 27, 2018

View reviewed changes

Show line number when there are multiple locations in the same file

84998d4

Trigger CI

9df1964

Comment out unless

10066a8

Add unless again and comment

6cd98aa

wooster0 changed the title ~~Prevent link duplicates in "Defined in:" section~~ Show line numbers at link when there are filename duplicates in "Defined in:" section (docs) Jun 29, 2018

Fix

f458bb5

Sija reviewed Jun 29, 2018

View reviewed changes

Don't serialize show_line_number

0ac1fd0

RX14 reviewed Jun 30, 2018

View reviewed changes

r00ster91 and others added 2 commits July 15, 2018 18:18

Fix

ae15ab9

Improve comment at unless

f2ebe89

RX14 reviewed Jul 16, 2018

View reviewed changes

Make show_line_number a property

6171005

RX14 approved these changes Jul 16, 2018

View reviewed changes

straight-shoota requested changes Jul 16, 2018

View reviewed changes

Use copy_with

f7b9934

Use class again

3b262c9

sdogruyol approved these changes Jul 28, 2018

View reviewed changes

sdogruyol merged commit 077920d into crystal-lang:master Jul 28, 2018

sdogruyol added the kind:docs label Jul 28, 2018

sdogruyol added this to the Next milestone Jul 28, 2018

RX14 added kind:feature topic:compiler topic:tools labels Jul 30, 2018

RX14 modified the milestones: Next, 0.26.0 Jul 30, 2018

wooster0 mentioned this pull request Aug 5, 2018

Fix identical link generation prevention for the "Defined in" section (docs) #6489

Merged

Show line numbers at link when there are filename duplicates in "Defined in:" section (docs) #6280

Show line numbers at link when there are filename duplicates in "Defined in:" section (docs) #6280

Conversation

wooster0 commented Jun 27, 2018 • edited

Defined in:

Defined in:

asterite left a comment

Choose a reason for hiding this comment

wooster0 commented Jun 27, 2018

asterite commented Jun 27, 2018

jhass commented Jun 27, 2018

wooster0 commented Jun 27, 2018

Defined in:

asterite commented Jun 27, 2018

wooster0 commented Jun 27, 2018 • edited

Defined in:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota commented Jun 27, 2018

wooster0 commented Jun 27, 2018

Defined in:

Defined in:

straight-shoota commented Jun 27, 2018

Defined in:

wooster0 commented Jun 27, 2018

straight-shoota commented Jun 27, 2018

wooster0 commented Jun 28, 2018 • edited

straight-shoota commented Jun 28, 2018

wooster0 commented Jun 28, 2018 • edited

straight-shoota commented Jun 28, 2018

RX14 commented Jun 29, 2018

wooster0 commented Jun 29, 2018

Choose a reason for hiding this comment

wooster0 Jun 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota left a comment

Choose a reason for hiding this comment

RX14 commented Jul 16, 2018

straight-shoota commented Jul 16, 2018

wooster0 commented Jul 24, 2018 • edited

RX14 commented Jul 28, 2018

sdogruyol left a comment

Choose a reason for hiding this comment

wooster0 commented Jun 27, 2018 •

edited

wooster0 commented Jun 27, 2018 •

edited

wooster0 commented Jun 28, 2018 •

edited

wooster0 commented Jun 28, 2018 •

edited

wooster0 Jun 29, 2018 •

edited

wooster0 commented Jul 24, 2018 •

edited