fizyr / subst

Apache License 2.0
10 stars 4 forks source link

substitute_one_step: stop after first variable has been substituted #11

Closed cre4ture closed 5 months ago

cre4ture commented 6 months ago

This change provides the posibility to only replace the next variable in a given string, returning the position after the variable name. This is usefull if the substitution is to be integrated into an existing string parsing statemachine. E.g. I'm integrating in into a "shell-words" like funktionality needed by the uutils/coreutils-env app.

To avoid that we need to do a fork of your code, we want to bring it into your main branch. Please tell me what you think.

Kind regards

de-vri-es commented 6 months ago

Hey! Sounds interesting. But maybe we can make it return a tuple of 3 slices instead of a Option<(String, usize)>: the leading part of the string before the next variable, the substitute value, and the remainder of the unprocessed string.

cre4ture commented 6 months ago

You where right, the interface was a bit mixed up. I did refactorings and I think this fits now more to your idea. What do you think?

de-vri-es commented 6 months ago

I think we still need to prevent returning a String, since it will re-allocate every time a change is made now.

But I also wonder, do you still need substitute_one(...) if we have a parse(..) -> Template function? Or can you achieve your desired end result already if parsing and performing substitution is split?

cre4ture commented 6 months ago

But I also wonder, do you still need substitute_one(...) if we have a parse(..) -> Template function? Or can you achieve your desired end result already if parsing and performing substitution is split?

I would still need a funktion like parse_one_template_part as I only trigger the variable substitution when I detected that there is a $. The part of the input after that variable is again parsed with my own parser.

But we can go for the template based approach anyway if you like. Just let me know what you thing about #14

de-vri-es commented 6 months ago

I would still need a funktion like parse_one_template_part as I only trigger the variable substitution when I detected that there is a $. The part of the input after that variable is again parsed with my own parser.

I think I'm not entirely seeing your use case clearly. It feels somewhat odd to perform only partial variable substitution. Wouldn't it make more sense to run your own parser, and then run the substitution on the relevant parts that your parser indicates?

Or maybe the opposite makes sense: parse a full template first, and then run your parser on the non-variable parts of the template?

Do you have a link to how you're using this maybe? Or otherwise could you explain with some pseudo-code how this is supposed to integrate with your parser?

But we can go for the template based approach anyway if you like. Just let me know what you thing about #14

I will! Probably not today though, as I'm on holiday :p So I have only limited time for reviews. But the comments about the unit tests apply to both PRs. If you have the time you could already process those.

cre4ture commented 6 months ago

This pull request contains (temporarily) the modified subst such that it works. If you like you can have a look at it: https://github.com/uutils/coreutils/pull/5801

I think the main problems I encountered when I tried to use the unmodified subst where these:

With the "one-step" way, I just need to stop at an unescaped $ and call subst_one_step which tells me about the end of the variable name and the substituted value.

In the beginning, when I didn't know the subst source code that well, for me the easiest way to achieve my goal was to provide access to this single/one-substitution step. Now, as I know your code pretty well, I detected that maybe the only function that I need from subst is the parse_variable. Making that function public woult probably be enought.

de-vri-es commented 6 months ago

Hmm, interesting. I still like the template approach, but I also like to unblock progress for uutils. I'll have a look at that PR.