-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Show line numbers at link when there are filename duplicates in "Defined in:" section (docs) #6280
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should prevent duplicates when recording the locations. That's in types.cr
I couldn't find any types.cr file which has to do with the docs so I did it in generator.cr now. |
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 |
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. |
@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) WDYT? |
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
Okay now the Defined in: section of for example Colorize would look like this: Defined in:colorize.cr 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. |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic should IMO be moved to the template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by template? Should I check if it's a duplicate at the type.html ECR file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's what I meant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't. Templates should be mostly logic-less. This way is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a duplicate
though (still different line numbers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use simple names. Use expressive names. As short as possible, though. I'd suggest something like multiple_locations_in_file
.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But don't I need to do this so I can use it in the type.html ECR file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. ECR templates directly access the instance methods of the Crystal object. This to_json
method is only for generating a JSON export.
src/compiler/crystal/types.cr
Outdated
@@ -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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The duplicate
flag should also be set on the other location.
I like the idea of showing the line numbers. But I'd suggest the format 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. |
I agree. Heres how the format on all links would look at for example Debug: Defined in:debug/elf.cr:1 And heres how it would look when only showing it at filename duplicates: Defined in:I vote for the first one. |
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.
|
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. |
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. |
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:
Maybe because of the |
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. |
Thats weird. test_darwin CI failed again with the same errors.
I'll comment out the Edit: It still fails. |
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 |
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. |
@RX14 Yeah that was a bad idea. I forgot to change the title and description of the PR. I changed it now. |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
show_line_number
-> show_line_number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it is a Bool
returning method, quite query-ish (show_...
) for my taste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we reduce the level of bikeshedding in comments? ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@show_line_number?
is not even a valid name for an instance variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry :/ @straight-shoota No, but it's still a valid name for an accessor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
locations << RelativeLocation.new(filename, location.line_number, url) | ||
show_line_number = locations.any? do |location| | ||
if location.filename == filename | ||
location.show_line_number = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this even work? location
is a struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats because of the setter
I added for show_line_number
in line 364.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(we need specs for the docs generator)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a property and remove it from the getter macro please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes that's a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from struct to class is unnecessary, this should work with record (see https://github.com/crystal-lang/crystal/pull/6280/files#r202628750).
@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. |
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. |
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. |
Another review from a core team member please, this is ready to go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @r00ster91 👍
For example this Defined in: section example:
Defined in:
colorize.cr
colorize.cr
foo.cr
bar.cr
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.