feat: provide span for more nodes #17
Conversation
|
Actually, going to convert to draft. I think all the Edit: Ok, should be good now. |
| @@ -27,6 +27,7 @@ lazy_static = "1.4" | |||
|
|
|||
| [dev-dependencies] | |||
| indoc = "0.3" | |||
| pretty_assertions = "0.7.2" | |||
There was a problem hiding this comment.
I will admit that it's a little bit overboard how many spans get tested with this PR, but this makes it a lot more manageable. Perhaps in the future we could make it so that spans can be skipped in some of the assertions.
There was a problem hiding this comment.
This is fine by me, I'd also like to reduce the test code verbosity eventually but I don't think we need to deal with that now.
|
I can't view the circleci output, so not sure why it's failing. Works fine for me locally (my guess would be maybe I used a new rust language feature? Seems like it's on rust 1.42) Edit: Yup, that was it. |
|
Hi! At first glance this looks great, though I'll need to circle back for a thorough review. One quick note, per the organization's code of conduct we do need a DCO sign-off. We'll squash and merge the commit, so assuming you agree to sign off on the DCO, if you add the string to the PR description I think GitHub will copy it into the squash commit's message (or we can copy it over manually on merge, whichever is needed). |
Signed-off-by: David Sherret <[email protected]>
dsherret
force-pushed
the
span-for-nodes
branch
from
f7cb7e2 to
c4cf236
Compare
|
@timothyb89 done! I squashed everything in this PR to a single commit and added that to the message. |
There was a problem hiding this comment.
On the whole, this looks great! I left a couple of small comments but otherwise I like this a lot.
Standardizing on SpannedString hurts ergonomics a bit but ultimately I think it's necessary.
I'd eventually like to figure out something to make the unit tests less verbose, but we don't need that in this PR. Maybe a SpannedString::quoted(offset: usize, content: &str) helper, or maybe something to mock spans where we don't care to rigorously re-validate them in every single comparison? In any case, a problem for another day.
| @@ -36,6 +53,67 @@ pub(crate) fn clean_escaped_breaks(s: &str) -> String { | |||
| s.replace("\\\n", "") | |||
| } | |||
|
|
|||
| #[derive(Debug, PartialEq, Eq, Clone)] | |||
There was a problem hiding this comment.
Small nit, this could use a docstring.
src/util.rs
Outdated
| impl SpannedString { | ||
| pub fn as_str(&self) -> &str { | ||
| self.content.as_str() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Now that we'll be using SpannedString everywhere, a couple quick impls would go a long way toward improving our API ergonomics:
impl AsRef<str> for SpannedString {
fn as_ref(&self) -> &str {
&self.content
}
}
impl fmt::Display for SpannedString {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
self.content.fmt(f)
}
}(there's a bunch of other trait impls that might make SpannedString pretend to be a regular str a bit better, but IMO these are the low-hanging fruit)
Signed-off-by: David Sherret <[email protected]>
|
Sorry, I completely forgot about this PR and didn't realize you left some comments. I have resolved them now I think. |
Several nodes in the tree were missing a span and there were a lot of
Stringproperties that would be better represented bySpannedString.I'm making a code formatter using this crate (seems like a very good parser 🙂) and so knowing all the spans is very important.
Closes #16