Skip to content
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

Allow nilable Spoom::Location lines and columns attributes #587

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

alexcrocha
Copy link
Contributor

Summary

This PR updates the Spoom::Location class to allow the start_line, end_line, start_column, and end_column attributes to be nilable. This change provides greater flexibility in representing different types of locations within a file.

Details

  • Nilable Attributes: The start_line, end_line, start_column, and end_column attributes of Spoom::Location can now be nil.
  • Flexible Spoom::Location Representation:
    • Can now represent an entire file, e.g., foo.rb.
    • It can also represent a range of lines within a file, e.g., foo.rb:1-3.
    • Maintains previous implementation, with both lines and columns, e.g., foo.rb:1:2-3:4.

What should Reviewers pay attention to

  • Exception Messages: Added more detailed messages to exceptions raised within the Spoom::Location class to improve debugging and error handling. Unsure if this is a good change.
  • Float::Infinity: Introduced Float::Infinity to represent inclusivity when lines or columns are nil, ensuring that ranges can be handled correctly even when the start or end is not explicitly known.

@alexcrocha alexcrocha requested a review from a team as a code owner July 19, 2024 19:34
@alexcrocha alexcrocha requested review from egiurleo and KaanOzkan July 19, 2024 19:34
@andyw8
Copy link
Contributor

andyw8 commented Jul 19, 2024

Are there perhaps two distinct concepts here that should be seperated out, e.g. LocationPosition and a LocationRange ?

@egiurleo
Copy link
Contributor

egiurleo commented Jul 22, 2024

Seconding Andy's question, and I was also wondering what the use case is here -- what are you planning to implement that uses this feature? NVM saw your other issue! Makes sense.

lib/spoom/location.rb Outdated Show resolved Hide resolved
lib/spoom/location.rb Outdated Show resolved Hide resolved
Comment on lines 24 to 27
start_line, end_line = T.must(start_line).split("-", 2)
return new(file, start_line: start_line.to_i, end_line: end_line.to_i) if end_line

raise LocationError, "Invalid location. End line is missing in range format: #{location_string}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer an early raise and an unconditional return:

Suggested change
start_line, end_line = T.must(start_line).split("-", 2)
return new(file, start_line: start_line.to_i, end_line: end_line.to_i) if end_line
raise LocationError, "Invalid location. End line is missing in range format: #{location_string}"
start_line, end_line = T.must(start_line).split("-", 2)
raise LocationError, "Invalid location. End line is missing in range format: #{location_string}" unless end_line
return new(file, start_line: start_line.to_i, end_line: end_line.to_i) if end_line

So the style and logic is coherent with the other branches.

lib/spoom/location.rb Outdated Show resolved Hide resolved
lib/spoom/location.rb Outdated Show resolved Hide resolved
lib/spoom/location.rb Outdated Show resolved Hide resolved
lib/spoom/location.rb Outdated Show resolved Hide resolved
lib/spoom/location.rb Outdated Show resolved Hide resolved
lib/spoom/location.rb Outdated Show resolved Hide resolved
lib/spoom/location.rb Outdated Show resolved Hide resolved
@Morriar
Copy link
Collaborator

Morriar commented Jul 22, 2024

Indeed, maybe the Location concept is not a good representation, maybe what we want is the concept of File vs. FileLocation. Though I'm wary of doing the refactoring right now as we are still exploring how we want to modelize whole file removals.

For now, the nilability change suits our needs and doesn't commit us to one direction or another 👍

Base automatically changed from ar/location-tests to main July 22, 2024 17:00
@alexcrocha alexcrocha force-pushed the ar/nilable-location branch from 1cdfd36 to 4163416 Compare July 22, 2024 21:02
@alexcrocha alexcrocha requested a review from Morriar July 22, 2024 21:09
Copy link
Collaborator

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

One small nit, we can merge once fixed 🎉

Comment on lines 31 to 32
if rest.nil?
raise LocationError, "Invalid location string `#{location_string}`: missing end line and column"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this fit in one line?

Suggested change
if rest.nil?
raise LocationError, "Invalid location string `#{location_string}`: missing end line and column"
raise LocationError, "Invalid location string `#{location_string}`: missing end line and column" if rest.nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, good catch! 🕵️

This change allows the start/end line/column attributes of a Location to
be nilable.
A `Spoom::Location` can now either be a whole file `foo.rb`, have start
and end lines `foo.rb:1-3`, or also include start and end columns
`foo.rb:1:2-3:4`.
@alexcrocha alexcrocha force-pushed the ar/nilable-location branch from 4163416 to c60632f Compare July 22, 2024 23:26
@alexcrocha alexcrocha merged commit 990d1a1 into main Jul 22, 2024
8 checks passed
@alexcrocha alexcrocha deleted the ar/nilable-location branch July 22, 2024 23:39
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