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

make get_variable usable from outside the crate #16

Closed
wants to merge 2 commits into from

Conversation

cre4ture
Copy link
Contributor

@cre4ture cre4ture commented Jan 9, 2024

No description provided.

@cre4ture cre4ture changed the title make get_varible usable from outside the crate make get_variable usable from outside the crate Jan 9, 2024
@de-vri-es
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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. :-)

@cre4ture cre4ture closed this Jan 12, 2024
@de-vri-es
Copy link
Contributor

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 :)

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 this pull request may close these issues.

2 participants