fizyr / subst

Apache License 2.0
10 stars 4 forks source link

make get_variable usable from outside the crate #16

Closed cre4ture closed 5 months ago

de-vri-es commented 6 months ago

I think I understand now what the end goal is, so I also understand why you want this.

I do have one concern though: the added public API is easy to use wrong, and exposing it publicly hinders future changes.

I'm almost inclined to expose a small wrapper for this function in a #[doc(hidden)] module (subst::hidden__::parse_variable()) so at-least we keep the freedom to change things in the main namespace (but still make semver promises for the hidden module).

For the errors, do you really need them to be Eq, PartialEq? It's not always obvious which details of an error should be included in comparisons.

On the other hand, we can see from our own unit tests that it is useful to be able to check errors. We could hide these derives behind a feature, maybe.

cre4ture commented 5 months ago

I added the Eq and PartialEq to the errors because I made them to be part of a own error type which I needed to compare in my tests. Apperently, the #[cfg(test)] works only within the same crate. But now, as you where questioning it, I removed the subst error from my error such that the Eq and PartialEq are not needed anymore.

But, lets talk about the more important thing: Would you agree on exporting the parse_variable? And can you point out an example how this works with the subst::hidden__?

I'm almost inclined to actually implement the parse variable directly in uutils. I'm finally not so sure anymore if its worth the additional dependency. Especially as subst seems to have a different focus: The template expansion rather then a shell script.

de-vri-es commented 5 months ago

But, lets talk about the more important thing: Would you agree on exporting the parse_variable? And can you point out an example how this works with the subst::hidden__?

Basically the same as it does now, just with a weird path excluded from the documentation. But still with semver guarantee.

I'm almost inclined to actually implement the parse variable directly in uutils. I'm finally not so sure anymore if its worth the additional dependency. Especially as subst seems to have a different focus: The template expansion rather then a shell script.

But you are right about this. subst is not fully compatible with shell script syntax. It only supports a fallback default values. No other type of variable substitution is supported, and it isn't really a goal of the project to be compatible with POSIX shell syntax, let alone bash or zsh.

So if you want things like ${var#prefix}, then subst won't really be sufficient.

cre4ture commented 5 months ago

Ok, I have now an own implementation. Sorry for the sudden mind change and for your efforts in the beginning. It was a constructive interaction with you. Thanks for this. I will close the pull-requests. Except if you say you want to have one of the "template parsing" implementations. Then I would support you to get it in. Just let me know. :-)

de-vri-es commented 5 months ago

No problem! I suspect it's the right choice if you want to go for POSIX compliance or any other specific format. I will look more at the template functionality later when I have the time :)