Improve the parameters passed into `BlobPresenter#hightlight`
We're currently using since
and to
, but from:
and to:
is probably better when refering to line numbers, not time.
The suggestion is to add a parameter object (can be named LineRange
or LineSelection
). We can then use and pass it along instead of passing 2 separate params.
The following discussions from gitlab-ce!31361 should be addressed:
-
@patrickbajao started a discussion: (+4 comments) Hi @alexkalderimis, can you please review this MR? Thanks!
🙂 -
@alexkalderimis started a discussion: (+1 comment) We also need a test for the
to:
parameter, wouldn't you agree? -
@alexkalderimis started a discussion: Is the expectation that these values should only be provided together? If I supply a since or a to, I would expect a performance speedup. Is that expectation met?
Also I am beginning to wonder if it would not be helpful to package these two values, which are related, into a parameter object, perhaps something like
LineSelection.new(since, to)
. This would allow things like:all_lines[selection.to_range]
and have meaningful methods such as
has_lower_bound?
andhas_upper_bound?
or even justis_bounded?
andis_all?
.Further to this, I have issues with the choice of
since
as a parameter name. "Since" is always used for time in English, and page/line numbers are always treated spatially. We say 'from line 2 to line 5'. Is there a reason why we should not use the appropriate preposition here? -
@alexkalderimis started a discussion: (+2 comments) This is the kind of expression that a parameter object (e.g.
LineSelection
orLineRange
) would make a bit nicer, allowing us to say:if bottom? && line_range.includes?(all_lines.size)
Obviously this includes more indirection, but I feel it might make things a bit clearer.
Thoughts?
-
@alexkalderimis started a discussion: see comment below about choice of parameter names and concept of a parameter object. It might be nicer to add a single parameter here rather than two.