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

feat: make Level::span generic over RangeBounds #152

Closed
wants to merge 1 commit into from

Conversation

SamWilsn
Copy link

@SamWilsn SamWilsn commented Oct 4, 2024

No description provided.

Bound::Included(e) => Bound::Included(e - new_start),
};
let range_end = match ann.range.1 {
Bound::Unbounded => Bound::Unbounded,
Copy link
Author

Choose a reason for hiding this comment

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

Hm, I'm wondering if this needs to be Bound::Exclusive(ann.exclusive_end(foo) - new_start) instead.

@Muscraft
Copy link
Member

Do you mind if I ask what the reasoning behind this change was?

@SamWilsn
Copy link
Author

Do you mind if I ask what the reasoning behind this change was?

Not at all! I tried to use a 0..=3 range, and noticed span wasn't generic over the types of ranges. This pull request improves the ergonomics of the span function so that it more closely matches the API of str.

@epage
Copy link
Contributor

epage commented Oct 15, 2024

Could you expand on what your use case is? We expect this to be used programmatically which would generally be done with one range type and the current is most flexible.

@SamWilsn
Copy link
Author

There is no use case. This is just an ergonomic upgrade. It allows the following ranges that weren't allowed before:

Level::Error.span(2..4);    // Range (what you support now)
Level::Error.span(2..=4);   // RangeInclusive
Level::Error.span(..4);     // RangeTo
Level::Error.span(..=4);    // RangeToInclusive
Level::Error.span(2..);     // RangeFrom
Level::Error.span(..);      // RangeFull

If this adds too much complexity, feel free to close. I don't mind.

@epage
Copy link
Contributor

epage commented Oct 16, 2024

What i want to understand from the use case is how you are creating the spans. Are you hand writing them? If so, why?

@SamWilsn
Copy link
Author

I have a lint that always starts from the beginning of a line, which is probably the only decent example I have.

@Muscraft
Copy link
Member

Without a more specific use case, I am not sure if the added complexity is worth it at this time. I think this is something to look into more as the library progresses.

That said, I am going to close this for now, but I greatly appreciate the PR.

@Muscraft Muscraft closed this Oct 23, 2024
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.

3 participants