Skip to content

Implement VariableMap for Vec<AsRef<str>> #31

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

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

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented May 9, 2025

This implements VariableMap for &[T] slices containing strings (T: AsRef<str>).

In addition to this, the parser is updated to allow a single * as variable name. This is used in above's VariableMap implementation to make $* and ${*} substitute to the concatenation of all variables but `$0, like a shell would do.

My use-case for this is too take an argument list like argv, and then use subst to replace those numbered arguments within a string.

@de-vri-es
Copy link
Contributor

de-vri-es commented May 9, 2025

Hey! Thanks for the PR.

The use case makes sense, but I'm not 100% sure if having a Vec<impl AsRef<str>> implicitly use the index as variable "name" makes sense. We could provide this functionality with a thin wrapper around a vec/slice as well.

On the other hand, I can't really think of another interpretation for using a vector as a map, so maybe it's better to implement it directly on Vec. But then we should also implement is for AsRef<str> slices :)

src/map.rs Outdated
Comment on lines 111 to 119
if key == "0" {
let mut s = String::new();
if let Some((first, rest)) = self.split_first() {
s.push_str(first.as_ref());
for v in rest {
s.push(' ');
s.push_str(v.as_ref())
}
}
Some(Cow::from(s))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this case seems a little odd: You seem to want "$0" to be used sort-of like "$@" is a shell?

This is quite surprising behavior to me though. Normally, "$0" would be the name of the program, not the concatenation of all arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, well spotted. I did have key == "*" there at some point. My use case doesn't have $0, but I wanted the numbering to start at 1, so I changed to this.

It probably makes sense to treat the Vec like a regular args vec (just returning the index, starting at 0), and using another key.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember why I went with 0: * is rejected by the parser as invalid variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmok, if making it possible to implement more shell-like behavior ($* returns all numbered args concatenated), I could add a special case for a name that's just `*, to the parser. Let me know!

@kaspar030
Copy link
Contributor Author

But then we should also implement is for AsRef<str> slices :)

Hm, doesn't Vec<AsRef<str>> DeRef to a slice anyways. Let me try that.

@kaspar030
Copy link
Contributor Author

Hm, doesn't Vec<AsRef<str>> DeRef to a slice anyways. Let me try that.

So only implementing this for &[AsRef<str>] slices just works for Vecs as well.

@kaspar030 kaspar030 force-pushed the variable_map_for_vec branch from de944a7 to e68c25d Compare May 9, 2025 16:55
@kaspar030
Copy link
Contributor Author

I've updated the parser and made this more shell-like.

@de-vri-es
Copy link
Contributor

Hey! Sorry for the delay. It's RustWeek in the Netherlands :)

Now, the PR does a few different things:

  1. Allow a special single character variable name: '*'
  2. Implement VariableMap for [impl AsRef<str>].
  3. Assign meaning to $* in that impl, which means: concatenate all other variables.

I think we can merge 2. But I have some reservations on 1 and 3. I think single symbols as variable names are cryptic. I don't particularly like the choice of shells to use them a lot. And regarding 3, it feels to specific to one use case to put it in the VariableMap impl for string slices.

I could see one way how this use case might be supported:

  • Allow more single-character variable names in subst, like '!', '@', '#', '%', '^', '*', maybe more. I'm not really a huge fan of this, as I still think they are rather cryptic. But if they do not have special meaning in subst itself, I think I could be convinced to allow them as variable names.
  • Implement a custom VariableMap in your project that wraps a string slice or vector to assign special meaning to the variables of your choice, such as: $* concatenates all variables.

@kaspar030
Copy link
Contributor Author

It's RustWeek in the Netherlands :)

I'm there, too! :) Maybe we bump into each other, are you going to the 10 years of Rust celebration?

I could see one way how this use case might be supported:

* Allow more single-character variable names in `subst`, like `'!'`, `'@'`, `'#'`, `'%'`, `'^'`, `'*'`, maybe more. I'm not really a huge fan of this, as I still think they are rather cryptic. But if they do not have special meaning in `subst` itself, I think I could be convinced to allow them as variable names.

* Implement a custom `VariableMap` in your project that wraps a string slice or vector to assign special meaning to the variables of your choice, such as: `$*` concatenates all variables.

I did a custom VariableMap (basically the version in this PR but over a newtype holding a [AsRef<str>]) in my project already, without the single letter $*/$@. Those I handle before passing to subst now. That kinda works but is a bit clumsy, I'm working on individual args (via shell_words::split()), and only support the single special letter variants for arguments that exactly match $*/$@/${*}/${@}, as replacing them using string replace would ignore escaping ...

I'd appreciate support for those special single letter variable names, to make this behave more like expected (by users used to shell).

If you'll have it, I'll create a new PR with just e68c25d (the commit allowing *), and update that to allow a list of special characters. Maybe behind a feature flag?

@de-vri-es
Copy link
Contributor

As also discussed at RustWeek: let's go ahead and support special characters as variable names: only single character special symbols, since that is what shells also do :)

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