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

v0.9 API #26

Closed
stoically opened this issue Oct 21, 2022 · 10 comments · Fixed by #40
Closed

v0.9 API #26

stoically opened this issue Oct 21, 2022 · 10 comments · Fixed by #40

Comments

@stoically
Copy link
Owner

stoically commented Oct 21, 2022

Follow-up from #23, also see v0.9.0-alpha.1 release notes

Issue for general API discussion of the upcoming v0.9 release, which will have a rather large breaking change upcoming: Node will be converted to an enum, and the node tree leafs are Node enums as well. This will come with additional type safety, but makes the nodes a bit harder to access in terms of API. Feel free to voice concerns or ideas.

Should NodeValueExpr implement Deref? Is it a newtype or a smart pointer?

After reading up a bit on what the book says about newtypes and smart pointers (here, here and here), I'm leaning towards NodeValueExpr being a smart pointer. Reason being: While it doesn't hold extra metadata, its main purpose is granting access to the type it is encapsulating and adding extra capabilities via a multitude of traits. Furthermore consumers mainly want work with the encapsulated type, so having to do explicit method calls just to get the Expr instead of being able to leverage reference coercion seems like the wrong move.

My understanding regarding the newtype patterns is that the main use cases are additional abstraction over encapsulated types – up to the point that it should protect internal type state from undesired modifications –, making APIs type safe through wrappers or specifically to work around the orphan rule. None if which is what NodeValueExpr is primarily doing/providing.

@Zizico2
Copy link

Zizico2 commented Oct 21, 2022

Ok, fair enough. I'm still on the fence about it, but I guess newtype vs smart pointers is not really a black vs white issue, it's more nuanced than that. And I like the renaming. NodeValue, to me, seemed like a newtype, and it being an Expr was just an implementation detail. NodeValueExpr makes it seem like it's an Expr (and it is), just with extra goodies.

@stoically
Copy link
Owner Author

but I guess newtype vs smart pointers is not really a black vs white issue, it's more nuanced than that.

Yeah, I'd agree.

NodeValueExpr makes it seem like it's an Expr (and it is), just with extra goodies.

That was the intention, glad it worked out. Also considered Value, ValueExpr and just Expr.

@stoically
Copy link
Owner Author

Something I realized: The impl TryFrom<NodeValue> for String isn't really discoverable in the rust docs?! It's really only discoverable through the examples. I wonder what a good way to expose that bit of information would be? Just some hand-written explanation in NodeValues doc comment? 🤔

@Zizico2
Copy link

Zizico2 commented Oct 23, 2022

Is it implemented?

Cuz

impl TryFrom<&NodeValueExpr> for String

this one shows up.

@stoically
Copy link
Owner Author

stoically commented Oct 24, 2022

Yeah, TryFrom<&NodeValueExpr>, but still, it isn't discoverable in the rustdocs, or am I blind? 🤔

@Zizico2
Copy link

Zizico2 commented Oct 24, 2022

TryFrom<&NodeValueExpr> shows up
TryFrom<NodeValueExpr> doesn't

@Zizico2
Copy link

Zizico2 commented Oct 24, 2022

You're looking at the alpha docs right?

@stoically
Copy link
Owner Author

@stoically
Copy link
Owner Author

stoically commented Nov 3, 2022

Released v0.9.0-beta.1.

v0.9.0 release scheduled for 10.11.2022.

@gbj
Copy link
Contributor

gbj commented Nov 15, 2022

I know the issue is closed, but just wanted to chime in on this and say thanks for the new API -- having just reimplemented our view macro using it and I can say it's much nicer to have the type safe enum variants than the constant .name_as_string().unwrap() and whatnot before. Great update!

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 a pull request may close this issue.

3 participants