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

Add a couple of non-canonical ctors #5

Merged
merged 1 commit into from
Mar 10, 2020
Merged

Add a couple of non-canonical ctors #5

merged 1 commit into from
Mar 10, 2020

Conversation

matklad
Copy link
Member

@matklad matklad commented Mar 9, 2020

No description provided.

src/range.rs Outdated
}

/// Creates a `TextRange` with specified starting offset and length.
pub fn offset_len(start: TextSize, length: TextSize) -> TextRange {
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not terribly happy about the naming here, but I do thing that we need a ctor with this signature. Doing TextRange(start, start + len) on the callsite doesn't work if start is a non-trivial expression.

@CAD97
Copy link
Collaborator

CAD97 commented Mar 9, 2020

Per discussion in #2, the construction patterns we want to make easy (but I'm up to changing these targets):

  • "of anchored text"
  • "of text at offset"
  • "empty at offset"
  • "start and end"

The currently implemented ways on master to achieve this:

  • "of anchored text" ⟹ TextRange(TextSize::zero(), TextSize::of(text))
  • "of text at offset" ⟹ TextRange(offset, offset + TextSize::of(text))
  • "empty at offset" ⟹ TextRange(offset, offset)
  • "start and end" ⟹ TextRange(start, end)

(TextSize::of may be of course replaced with whatever text.len() the text object provides.)

With this PR, we have:

  • "of text at offset" ⟹ TextRange::offset_len(offset, TextSize::of(text))
  • "empty at offset" ⟹ TextRange::empty(offset)

Counter proposal 1 – TextRange::anchored:

  • "of anchored text" ⟹ TextRange::anchored(TextSize::zero(), text)
  • "of text at offset" ⟹ TextRange::anchored(offset, text)

Counter proposal 2 – TextRange::offset, which is needed anyway to reintroduce Add::add functionality:

  • "of anchored text" ⟹ TextRange::of(text)
  • "of text at offset" ⟹ TextRange::of(text).offset(offset)

Counter proposal 3 – factory style length replacement TextRange::with_len:

  • "of text at offset" ⟹ TextRange::at(offset).with_len(TextSize::of(text))
  • "empty at offset" ⟹ TextRange::at(offset)

I think TextRange::at(offset) reads marginally better than TextRange::empty(offset), though the latter is arguably more intent-clear. I agree we need an "offset_len", but I'm not willing to accept that name without exploring the design space a bit more.

@matklad
Copy link
Member Author

matklad commented Mar 10, 2020

I do feel that TextRange::empty is much better than TextRange::at. To me, the latter makes sense only with combination with with_len (and that calls for a dedicated at_with_len or somthing function).

I agree we need an "offset_len", but I'm not willing to accept that name without exploring the design space a bit more.

I now think that, with TextRange::empty, I wouldn't miss offset_len that much. Like, we still need it, but we can make progress without.

@matklad matklad merged commit ea66e56 into master Mar 10, 2020
@matklad
Copy link
Member Author

matklad commented Mar 10, 2020

Hm, I am starting to think that maybe TextRage::offset_len(start, len) should be spelled as TextRange::xxx(len) + start, but I don't know a good name for xxx. Like, it should be the dual of empty, where start, and not length, is zero. TextRange::to(offset)?

@kjeremy
Copy link

kjeremy commented Mar 10, 2020

with_offset?

@matklad
Copy link
Member Author

matklad commented Mar 10, 2020

Are you talking about my xxx? That should be exactly the opposite, with_len

@matklad matklad deleted the ctors branch March 10, 2020 13:49
@CAD97
Copy link
Collaborator

CAD97 commented Mar 10, 2020

On a previous branch, I had it as TextRange::of(text: impl TextSized) => TextRange(0..text.len()). I don't know how obvious that naming is anymore, but just to throw it back out there.

@matklad
Copy link
Member Author

matklad commented Mar 10, 2020

I am more concerned not with the naming, but with the fact that the argument should be a TextSize and not impl TextSized.

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

Successfully merging this pull request may close these issues.

3 participants