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

Node method to get span for the whole node #14

Closed
stoically opened this issue Jan 7, 2021 · 7 comments · Fixed by #42
Closed

Node method to get span for the whole node #14

stoically opened this issue Jan 7, 2021 · 7 comments · Fixed by #42
Labels
enhancement New feature or request nightly

Comments

@stoically
Copy link
Owner

stoically commented Jan 7, 2021

This should probably span all children nodes as well? Like in the case of e.g. <><div /></> the fragment span should be 0 to 12? Is that what you'd expect from such method, @zetanumbers?

If possible add a field containing the span to node types.

@stoically stoically added the enhancement New feature or request label Jan 7, 2021
@zetanumbers
Copy link

I will link to this issue this inside of my code. Thank you.

@zetanumbers
Copy link

zetanumbers commented Jan 7, 2021

Btw I saw for example NodeName straightforward implementing span method. You should consider implementing the syn::spanned::Spanned trait instead.

@stoically
Copy link
Owner Author

Btw I saw for example NodeName straightforward implementing span method. You should consider implementing the syn::spanned::Spanned trait instead.

Thanks for the hint. While reading https://docs.rs/syn/latest/syn/spanned/trait.Spanned.html, I realized that Spanned is automatically implemented if ToTokens is implemented, which is already the case for NodeName. So I can just drop the manual span implementation.

@stoically
Copy link
Owner Author

stoically commented Oct 24, 2022

It seems to implement a span method for a whole node it'd need the https://docs.rs/proc-macro2/latest/proc_macro2/struct.Span.html#method.join method, which currently is still nightly only, since it depends on rust-lang/rust#54725.

Unless I'm missing a different way to construct a span from a start and end span.

@danheuck
Copy link
Contributor

While this can't be implemented properly in stable until Span::join() is stabilized, it would be nice for diagnostic purposes to be able to get some span for every kind of node even if it doesn't actually cover the entire node. I think the worst case here is fragment, but even there we can default to the start of the opening tag which definitely beats using Span::call_site(). Happy to attempt a PR if you want.

@stoically
Copy link
Owner Author

@danheuck I like the idea and would be happy to review a PR for it. This could probably also help with #12, where the span would help with formatting custom errors.

@stoically
Copy link
Owner Author

Only thing I'm a bit unsure about is in which cases it actually makes sense to store an additional span, given that the Expressions already give access to spans.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request nightly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants