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

Created as_span method #224

Merged
merged 3 commits into from
Apr 11, 2018
Merged

Created as_span method #224

merged 3 commits into from
Apr 11, 2018

Conversation

sunjay
Copy link
Contributor

@sunjay sunjay commented Apr 10, 2018

Fixes #223.

I left into_span so that this change is backwards compatible. into_span is now implemented in terms of as_span because they do the same thing.

Since the two methods are mostly the same, the test is also pretty much exactly the same. I added an extra part at the end to make sure that the pair is not consumed by the method.

@jstnlef
Copy link
Contributor

jstnlef commented Apr 10, 2018

We're already breaking all sorts of compatibility with 2.0.0 on the horizon. Not sure I would bother too much with maintaining compatibility. Looks good otherwise though 👍

@@ -123,6 +123,35 @@ impl<'i, R: RuleType> Pair<'i, R> {
/// ```
#[inline]
pub fn into_span(self) -> Span<'i> {
// into_span was here first, so it is left in for backwards compatibility
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to mark this as deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could replace the documentation with a message to mention that it is deprecated. There is currently no better way to do that in Rust.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid there might be. Seems to be stabilized too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Yes you are right. I was thinking of the stability attributes.

self.as_span()
}

/// Returns the `Span` defined by the `Pair`, **without** consuming it.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep the documentation the same for both. The only difference is that the deprecated one should point here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure no problem

@dragostis
Copy link
Contributor

@jstnlef I would keep backwards compatibility here since this is a pretty popular method and it's pretty simple to deprecate.

@sunjay
Copy link
Contributor Author

sunjay commented Apr 10, 2018

I deprecated into_span and replaced its occurrences with as_span. No more need for an explicit clone. :)

The docs for as_span are now exactly the same as the original documentation for into_span. The documentation for into_span contains a note that the method is deprecated. This is in addition to the one that will be generated by the Rust compiler whenever that method is used.

Copy link
Contributor

@dragostis dragostis left a comment

Choose a reason for hiding this comment

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

LGTM modulo the versioning change!

@@ -122,7 +124,34 @@ impl<'i, R: RuleType> Pair<'i, R> {
/// assert_eq!(pair.into_span().as_str(), "ab");
/// ```
#[inline]
#[deprecated(since="1.0.7", note="Please use `as_span` instead")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change to 2.0.0.

@sunjay
Copy link
Contributor Author

sunjay commented Apr 11, 2018

Done. :)

@dragostis dragostis merged commit 7eab574 into pest-parser:master Apr 11, 2018
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