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

Coalesce-like string function #2379

Open
faultymajority opened this issue Sep 20, 2024 · 8 comments
Open

Coalesce-like string function #2379

faultymajority opened this issue Sep 20, 2024 · 8 comments

Comments

@faultymajority
Copy link

In a Justfile I'm building, I have use for "optional arguments". It would be helpful to have something like

target a *b:
  echo {{ coalesce(b, a) }}

where the return value of coalesce is b is passed (i.e., its value has length > 0), and a otherwise. In that way, b would be an optional argument, with a as default.

Would a PR for this be welcome? The function name would probably need bike shedding, as SQL coalesce operates based on the first argument being NULL, not "empty".

It would (for my use case) be even nicer if there were syntax

target a *b=a:
  echo {{ b }}

which currently fails with "error: Variable a not defined". Could this ever work? As the pieces seem there:

x := "abc"
target a *b=x:
  echo {{ b }}

UDFs in general
Generally - for "user-defined functions" I'm currently using the following pattern

coalesce a *b:
  @echo {{ a }}
target a *b:
  echo $(just coalesce {{ b }} {{ a }})

Is this the intended way to have UDFs?

@laniakea64
Copy link
Contributor

It's already possible:

target a *b:
  echo {{ if b == '' { a } else { b } }}

It would (for my use case) be even nicer if there were syntax

target a *b=a:
  echo {{ b }}

which currently fails with "error: Variable a not defined". Could this ever work?

Seems it does work?

a := ''
target a *b=a:
  echo {{ b }}
$ just target 123
echo 123
123
$ just target 123 456
echo 456
456

@faultymajority
Copy link
Author

faultymajority commented Sep 20, 2024

It's already possible:

target a *b:
  echo {{ if b == '' { a } else { b } }}

Good to know, although this is a bit repetitive.

Seems it does work?

a := ''
target a *b=a:
  echo {{ b }}

Oh this is interesting - it would be a bit nicer not to have to declare a in the global namespace, but I wouldn't have guessed this, thanks!

@casey
Copy link
Owner

casey commented Sep 21, 2024

Oh this is interesting - it would be a bit nicer not to have to declare a in the global namespace, but I wouldn't have guessed this, thanks!

lmao I have no idea why you have to declare a, this is definitely not intended behavior.

@casey
Copy link
Owner

casey commented Sep 21, 2024

Opened an issue to figure this out #2381.

@casey
Copy link
Owner

casey commented Sep 21, 2024

As of #2382, you can now use earlier recipe parameters in later recipe parameter default values, so this will now work:

target a *b=a:
  echo {{ b }}

This was so close to working, the fix was very small, just two trivial changes in one place, and there was even a test which checked that it was an error. I'm really hoping that past me didn't have some good reason for not allowing this.

The only reason I can think to not allow this is if you want to reference an outer variable but can't because you're shadowing it with a prior paramter. However, we already had shadowing, as @laniakea64 discovered (how did you figure this out btw?), which we couldn't remove without a backwards-incompatible change, so I can't really think of a reason not to enable it.

@casey
Copy link
Owner

casey commented Sep 21, 2024

Is this the intended way to have UDFs?

Not really, but only because there isn't any way to create user-defined functions. #1059 tracks adding user-defined functions, although there's been no movement on it for a while since it's hard. I started a PR to add it but then got demoralized when I realized it would be tricky 😂

Should I leave this issue open or does allowing parameters to reference each other work for your use case? A coalesce function wouldn't be too hard to add, although it seems like it might have limited applicability.

@laniakea64
Copy link
Contributor

we already had shadowing, as @laniakea64 discovered (how did you figure this out btw?)

By being about to answer "this would break backwards compatibility with using the global variable a in that position" to this issue, was going to include an example to demonstrate what was happening, but running the example showed that answer would've been completely wrong 😅

@casey
Copy link
Owner

casey commented Sep 21, 2024

Very nice catch!

I just bisected and figured out what happened. Originally, having a parameter shadow a global variable was forbidden. Then it was allowed, but uses of that parameter name would always refer to the global variable, even if a previous parameter had the same name.

Then, in #2138, which refactored the evaluator, this behavior changed, and uses of that variable name in default values would refer to the previous parameter.

This was due to a somewhat subtle change, from this:

let mut scope = scope.child();
…
scope.bind(parameter.export, parameter.name, value);

To this:

let mut evaluator = Self::new(context, context.scope);
…
evaluator
  .scope
  .bind(parameter.export, parameter.name, value);

So now, parameters are defined directly in the evaluator scope, instead of a separate scope, where they can now affect the evaluation of following parameters.

This change was not actually made by the author of the PR, @neunenak, who is wholly innocent, but by me, in some random commits I piled on afterwards.

I looked at the rest of the PR, and this is the only place with such a change, so there probably weren't any other unexpected changes to evaluation scope.

This was an unintentional backwards-incompatible change, with the first version with the new behavior being 1.29, published on 6/13/2024, or a little under three months ago.

So, this is a somewhat interesting scenario in that we have a fairly recent, unintentional backwards incompatible change.

just's policy is to not make backwards incompatible changes on purpose. But if they're made accidentally, it shouldn't be the case, necessarily, that they're reverted. If a change is neutral or beneficial, and more disruption would be caused by reverting it, we should probably let it be.

This change seems beneficial to me. With it, if you want to access a global variable in a default value, you can simply use a different name for the previous parameter that shadows it. However without it, there is no way to access the value of a previous parameter.

It would be great to know how many users are on previous versions, since if there were large numbers of users on 1.28 or earlier, then it is conceivable that, if the change were disruptive enough, reverting it would be a net benefit.

Looking at the to packaging status tables:

https://repology.org/badge/vertical-allrepos/rust:just.svg

https://repology.org/badge/vertical-allrepos/just.svg

The largest distros that are on 1.28 and earlier are nixpkgs stable, Alpine, Guix, and Ubuntu 24.04 LTS. All the rest are on 1.29 or later.

So I think that since this is mildly beneficial, most users are on newer versions, it would probably be more disruptive to revert than not, and we haven't actually gotten any bug reports about it, we should probably just let it ride.

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

No branches or pull requests

3 participants