refactor!: switch Node to enum-style

[stoically]

Goals

• Avoid that consumers have to manually check the documentation in order to know when it is safe to unwrap node names/values
• Make the API feel more rusty

Notes

• Decided against the NodeElementAttribute / NodeElementChild enums as it would complicate the API significantly
• The price for less complicated node name/value unwrapping – and hence more type safety – is a slightly harder to use API, but well worth it

Remaining work

• Add TryInto<T> implementations on node types to get a specific syn expression for node values, such as TryInto<Expr::Lit> for NodeText
• Fix tests

Feedback welcome.

Resolves #16, Resolves #15

[Zizico2]

Let me know if I can help. I agree with everything you said. And about the duplication part, I'm not that familiar with syn-rsx's internals, but we can implement the common stuff on the enum directly, no? And maybe have the different structs implement a common trait.

#16 (comment)

I see the remaining work checklist. I might give some of those a crack, in the coming week.

[codecov-commenter]

Codecov Report

Merging #23 (419a465) into main (0f13493) will decrease coverage by 10.33%.
The diff coverage is 49.09%.

@@             Coverage Diff             @@
##             main      #23       +/-   ##
===========================================
- Coverage   84.74%   74.40%   -10.34%     
===========================================
  Files           4        4               
  Lines         367      379       +12     
===========================================
- Hits          311      282       -29     
- Misses         56       97       +41     

Impacted Files	Coverage Δ
src/lib.rs	58.33% <ø> (-10.90%)	⬇️
src/node.rs	33.33% <34.88%> (-13.89%)	⬇️
src/parser.rs	94.06% <100.00%> (-0.71%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[stoically]

I see the remaining work checklist. I might give some of those a crack, in the coming week.

Thanks, but it's already done. I'd be happy about a review tho. The node enum makes things slightly harder to access, but overall it feels more stable and rusty now I'd say.

[Zizico2]

Imma migrate part of my project to the node-enum branch and report back.

[stoically]

Force-push implements AsRef<Expr> for NodeValue so it gets easier to work with node value: less required usage of &*.

[Zizico2]

Force-push implements AsRef<Expr> for NodeValue so it gets easier to work with node value: less required usage of &*.

This is very welcome. I'm already using it.

[stoically]

Force-push renames NodeValue::value to NodeValue::expr, since a) that's what it actually is and b) if you have to access the expression directly, it's now node.value.expr instead of node.value.value.

Also adds From<Expr|ExprLit|ExprBlock> for NodeValue, primarily for internal parser usage.

[Zizico2]

makes sense. I was gonna suggest this. This may be futile by me, but value.value really annoyed me visually. And isn't implementing Deref and having the single field be public for newtypes considered bad?

Could the expr be made private and the Deref, implementations removed? And then implement From<&NodeValue> and From<NodeValue> for syn::Expr?

Maybe even pass Expr's methods through but I feel like that's not really needed since you should never really need to mutate the expr in place. But I could be wrong.

EDIT: removed wrong DerefMuts

EDIT: This kinda goes against what I said in the previous comment. And it might require expr to be cloned more times than strictly necessary. IDK, it's just a thought as I personally avoid implementing Deref and having a public field for newtypes.

EDIT: The striked sentence above isn't true as you can implement From<NodeValue> for &syn::Expr and From<&NodeValue> for &syn::Expr.

[stoically]

Right, NodeValue is a smart pointer and those generally don't expose the actual type directly. I wonder why that's an anti-pattern tho. 🤔 In any case, force-push makes expr private.

I wonder tho, why would you prefer the From<T> implementations over the Deref one? The latter seems to be an often used pattern for smart pointers?

[stoically]

Force-push also adds From<NodeValue> for Expr and From<&'a NodeValue> for &'a Expr, because having more ways to get to the value.expr can't hurt either way, right?

[Zizico2]

Force-push also adds From<NodeValue> for Expr and From<&'a NodeValue> for &'a Expr, because having more ways to get to the value.expr can't hurt either way, right?

Yeah

I wonder tho, why would you prefer the From<T> implementations over the Deref one? The latter seems to be an often used pattern for smart pointers?

I guess I am not considering NodeValue a smart pointer. I'm considering it a newtype: it only really wraps Expr for semantics and for the implementation of traits.

And for newtypes implementing Deref is an anti-pattern.

Could you create a discussion for general architecture?
This isn't really related to the Node enum anymore.
I'm developing a web framework, heavily influenced by Qwik (resumability) and Lit (struct based, like Lit is class based). It's just a toy project, it's not even public yet. But it is complex enough that I feel I could have further suggestions.

[stoically]

Force-push renames NodeValue to NodeValueExpr so it's easier to recognize its intended purpose.

@Zizico2

Could you create a discussion for general architecture?

Did so here: #26

I guess I am not considering NodeValue a smart pointer. I'm considering it a newtype: it only really wraps Expr for semantics and for the implementation of traits.

I feel like it's a tricky topic, but picked it up in the linked issue above. Happy to discuss that further.

I'm developing a web framework, heavily influenced by Qwik (resumability) and Lit (struct based, like Lit is class based). It's just a toy project, it's not even public yet. But it is complex enough that I feel I could have further suggestions.

Sounds interesting! I'd be interested to take a look once you publish something.

[stoically]

Released as v0.9.0-alpha.1