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

feat!: Add partial and permissive substitutions and recursive braced variables substitution #18

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

D-Brox
Copy link
Contributor

@D-Brox D-Brox commented May 1, 2024

Allow three modes of substitution:

  • Strict is identical to the previous behavior.
  • Partial only substitutes the variables that are in the variable map or that have defaults, keeping the ones that aren't defined.
  • Permissive replaces the variables with an empty string if they are not in the variable map.

This PR also fixes recursive substitution to allow braced variables, so ${va1:${var2}} is now also allowed. Cherry-picked to main in 476997b

This is a breaking change, as the function signatures have changed.

@D-Brox D-Brox changed the title feat!: Add partial and permissive substitutions feat!: Add partial and permissive substitutions and recursive braced variables substitution May 1, 2024
@D-Brox
Copy link
Contributor Author

D-Brox commented May 2, 2024

I think I may need to add more to the documentation

@de-vri-es
Copy link
Contributor

de-vri-es commented Jun 9, 2024

Thanks for the PR, and sorry for the delay!

I've cherry-picked the fix to main and released it as v0.3.1.

For the mode changes: I do like the idea of allowing the user to choose what to do on missing variables.

But there is a bit of murky territory: If you have "${a:${b:$c}}", and none of the variables are defined, what do we expect Partial to do here? As a user, I think I would expect the string to be untouched, but in practise this will probably result in "$c"? (Which is much easier in the implementation and arguably still correct, possibly even desirable.)

I also don't want to break backwards compatibility for adding this. To avoid that, I see two ways forward:

  1. Extend the VariableMap trait to allow it to make this choice. Then users can wrap their variable map in a wrapper type to change the way undefined variables are handled.

  2. Implement a Template struct that holds a parsed template, and where the user can set this mode before performing substitution. Added benefit is that it allows for more efficient expansion of the same string twice.

    The existing functions would remain as short-hand for instantiating a template and performing expansion.

    But I have some pretty specific requirements in mind for this struct, so please don't start on it without discussing first :)

@D-Brox
Copy link
Contributor Author

D-Brox commented Jun 11, 2024

I see, there really is ambiguity in the Partial mode. The current implementation would indeed replace ${a:${b:$c}} with $c. Maybe we could add a Lazy mode that leaves it untouched, making the distinction more clear.

About your suggestions, 1. should be easier and faster to implement, but I personally like the idea in 2.

@de-vri-es
Copy link
Contributor

de-vri-es commented Jun 16, 2024

I added re-usable templates in #21. Maybe you can also look at that PR to evaluate if there is anything you think should be different to add multiple expansion modes in the future.

@D-Brox D-Brox marked this pull request as draft June 21, 2024 14:25
@D-Brox
Copy link
Contributor Author

D-Brox commented Jun 21, 2024

I'll look at it over the weekend

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