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

Implement Span::sub_span and Span::lines_span. #456

Closed
wants to merge 5 commits into from

Conversation

tangmi
Copy link

@tangmi tangmi commented Apr 25, 2020

Prototype implementation for #455

@CAD97
Copy link
Contributor

CAD97 commented Apr 25, 2020

This looks reasonable to me at a glance. @dragostis ?

@tangmi
Copy link
Author

tangmi commented Apr 25, 2020

Some concerns I have (that may not actually be real issues):

  • Is Span intended to be consumable by users in this way? My assumption is that pest expects/allows users to create Span, given that Span::new is exposed (although I'm proposing this PR because Span::new is difficult to use with a parsed document since Span::input is not exposed).
  • I didn't put much thought into the naming of new members
    • Span::sub_span would be called Span::get to parallel str::get (there is no "substring" method in std, just range indexing)
    • LinesSpan was just named as such to avoid changing the name of Lines.

@Alextopher
Copy link
Contributor

I am also interested in sub_span. I am also happy to provide some kind of "help wanted" to see it through

@Alextopher
Copy link
Contributor

Alextopher commented Aug 3, 2022

Is Span intended to be consumable by users in this way?

There are similar requests for additional flexibility ( such as #469 ). There are other types that provide flexibility such as ErrorVariant::CustomError. Subspans could be similarly useful.

Span::sub_span would be called Span::get to parallel str::get

I prefer Span::get to match other subslicing behaviors

LinesSpan seems fine to me, it will probably only ever be used in the context of the iterator. I don't think the type needs a different name.

@tomtau is this PR blocked waiting for better documentation?

@tomtau
Copy link
Contributor

tomtau commented Aug 3, 2022

@Alextopher unless @tangmi plans to revive this PR soon, yes, I think two main items to fix will be to adjust the docs a little bit (to provide a full example usage) and to resolve a minor conflict with the latest target branch.
@Alextopher feel free to open a new PR based on this one.

Comment on lines +69 to +80
let start = match range.start_bound() {
std::ops::Bound::Included(&offset) => offset,
std::ops::Bound::Excluded(&offset) => offset + 1,
std::ops::Bound::Unbounded => 0,
};
let end = match range.end_bound() {
std::ops::Bound::Included(&offset) => Some(offset + 1),
std::ops::Bound::Excluded(&offset) => Some(offset),
std::ops::Bound::Unbounded => None,
};
let s = self.as_str();
if s.get(start..end.unwrap_or(s.len())).is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Alextopher one more item to fix: the merge conflict seems to be about core vs std. pest now supports no_std, so these ops may either need to be adjusted for no_std or sub_span should be feature-guarded for std

Comment on lines +308 to +310
Some(unsafe {
Span::new_unchecked(self.span.input, line_start, self.pos.min(self.span.end))
})
Copy link
Contributor

Choose a reason for hiding this comment

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

@Alextopher could this be done without unsafe? if not, it should have a Safety comment explaining why it is safe or when things could go wrong

@Alextopher Alextopher mentioned this pull request Aug 3, 2022
@Alextopher
Copy link
Contributor

Sounds good. I started by creating a new PR with just Span::get and will start looking into LinesSpan now.

@tomtau
Copy link
Contributor

tomtau commented Aug 3, 2022

closing in favour of #681

@tomtau tomtau closed this Aug 3, 2022
@Alextopher Alextopher mentioned this pull request Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants