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

Add support for [alias(name)] attribute #2344

Closed
wants to merge 3 commits into from

Conversation

marcaddeo
Copy link
Contributor

This adds an [alias(name)] attribute to Just, allowing aliases to be set in two ways.

alias f := foo

foo:
  echo "foo"

[alias(b)]
bar:
  echo "bar"

The alias attribute will inherit the private attribute from it's associated/target recipe.

[alias(b)] # Alias `b` will be private
[private]
bar:
  echo "bar"

This will need some additional tests and documentation of course, but figured I'd submit it first to make sure it's a desirable feature/implemented alright.

@laniakea64
Copy link
Contributor

Why?

@marcaddeo
Copy link
Contributor Author

Why?

I prefer the attribute syntax to assign metadata to a recipe over the alias keyword, seems more compact to me and keeps all the recipe details in one place.

Why not?

@casey
Copy link
Owner

casey commented Sep 5, 2024

I think I'm also a bit skeptical. I don't see a huge benefit in being able to declare aliases multiple ways, and since aliases are very user-facing, I think the dedicated item syntax makes them more discoverable.

@marcaddeo
Copy link
Contributor Author

I think I'm also a bit skeptical. I don't see a huge benefit in being able to declare aliases multiple ways, and since aliases are very user-facing, I think the dedicated item syntax makes them more discoverable.

Doesn't being able to alias f := foo anywhere in the Justfile make them less discoverable? Whereas the attribute has to be associated with the recipe it's aliasing.

There certainly isn't a huge benefit to this addition, I just thought it'd be a nice way to be able to set aliases. Or at least it's the way I'd prefer to be able to 😅

@laniakea64
Copy link
Contributor

Personal preference of someone other than @casey is not a rationale for a just code change.

Why not?

This new way to create aliases doesn't add additional functionality, while it does add (some) maintenance burden.

To maximize the space for just to grow while retaining its strong backwards compatibility guarantee, just intentionally maximizes how much justfile input is invalid. Because something that is an error in current just can be used for a new or improved feature later, but something that works in current just needs to be kept that way: it would be backwards-incompatible to later make it not work or behave differently. So best avoid adding syntaxes that only duplicate existing functionality.

And alias doesn't even make sense as an attribute. Attributes change recipe behavior. Aliases only provide alternative ways to invoke recipes from the command line, the recipe has the same behavior as if invoked by its real name.

Doesn't being able to alias f := foo anywhere in the Justfile make them less discoverable?

Nope, justfile authors aren't going to intentionally disorganize their justfiles for the purpose of making it harder for them to manage their own justfiles. And for people who run justfiles, this is moot since they'll just look in just --list.

@marcaddeo
Copy link
Contributor Author

Why?

I prefer the attribute syntax to assign metadata to a recipe over the alias keyword, seems more compact to me and keeps all the recipe details in one place.

Why not?

@laniakea64 sorry, this was rude. I interpreted your "Why?" as being rude here, but I should not have. I'm realizing I really didn't provide enough context in the original request. Let me try again.

I think it would be nice to have the ability to set aliases for a recipe with an attribute. I find that it keeps the information about a recipe more condensed and visually parseable when reviewing a recipe. I find it cleaner than setting an alias elsewhere in the file using the keyword.

@marcaddeo
Copy link
Contributor Author

And alias doesn't even make sense as an attribute. Attributes change recipe behavior. Aliases only provide alternative ways to invoke recipes from the command line, the recipe has the same behavior as if invoked by its real name.

I see. For some reason I thought a recipe could use aliases as dependencies, and that's what the [private] attribute on aliases was for. But that isn't the case, so at the very least inheriting it from the recipe probably doesn't make sense.

This new way to create aliases doesn't add additional functionality, while it does add (some) maintenance burden.

To maximize the space for just to grow while retaining its strong backwards compatibility guarantee, just intentionally maximizes how much justfile input is invalid. Because something that is an error in current just can be used for a new or improved feature later, but something that works in current just needs to be kept that way: it would be backwards-incompatible to later make it not work or behave differently. So best avoid adding syntaxes that only duplicate existing functionality.

These are absolutely good points, and I may have gotten a bit ahead of myself here. Probably should have started with an issue to get feedback first.

@neunenak
Copy link
Contributor

neunenak commented Sep 5, 2024

For what it's worth, I think that having a syntax to define an alias immediately next to the recipe it's an alias for is convenient. If the alias name := recipe syntax didn't already exist, I think I'd prefer this attribute-based syntax in fact. I agree that an alias is more discoverable if it's defined in the same block of code as the recipe itself, rather than (potentially) many lines away in a justfile or in a different justfile file altogether, which the alias name := recipe syntax currently allows.

It's also the case that there's a few places in just where there are multiple syntaxes for doing the same thing, generally because one syntax was added later to support some new feature (e.g. recipe documentation can be defined either with a comment above the recipe or with the [doc] attribute). So I don't think it's crazy to allow both the alias item and an alias attribute.

And alias doesn't even make sense as an attribute. Attributes change recipe behavior. Aliases only provide alternative ways to invoke recipes from the command line, the recipe has the same behavior as if invoked by its real name.

Some uses of attributes are better characterized as adding metadata to a recipe than changing its behavior - [group] and [doc] and [private] in particular don't change how the recipe works, but do change how it gets listed. An [alias] attribute feels conceptually pretty similar to these attributes.

Nope, justfile authors aren't going to intentionally disorganize their justfiles for the purpose of making it harder for them to manage their own justfiles. And for people who run justfiles, this is moot since they'll just look in just --list.

Something that I've noticed when working with large, multi-module justfiles maintained by teams of people, is that it's possible for an alias to be defined immediately before or after the recipe it aliases initially, but then for that alias definition to become separated from the recipe when someone else adds a new recipe to a file without taking notice of the alias (since they're not working on that recipe at the time). I don't think this is that big of a deal, because as you say just --list will always let users know what the aliases are, but it's something that users using this alias-attribute-syntax wouldn't run into.

@laniakea64
Copy link
Contributor

For what it's worth, I think that having a syntax to define an alias immediately next to the recipe it's an alias for is convenient.

It already exists. The existing alias foo := recipe syntax can already be positioned right before or right after a recipe.

there's a few places in just where there are multiple syntaxes for doing the same thing, generally because one syntax was added later to support some new feature

(bolding mine)
Exactly, and that isn't the case here. The proposed [alias] attribute and identifier attribute argument don't pave the way for new functionality.

If the [alias] attribute took string literal arguments like all other attributes' arguments, it would at least in theory open the door to e.g.

  • non-identifier aliases (for example using ! as alias, being able to invoke a recipe like just !)
  • dynamically-generated alias names if/when attribute arguments are allowed to be expressions

... and the time to add [alias] attribute would be when introducing such new functionality, to maximize flex in tailoring it appropriately.

Something that I've noticed when working with large, multi-module justfiles maintained by teams of people, is that it's possible for an alias to be defined immediately before or after the recipe it aliases initially, but then for that alias definition to become separated from the recipe when someone else adds a new recipe to a file without taking notice of the alias (since they're not working on that recipe at the time).

Wouldn't a justfile linting tool be a more effective way to address that?

@laniakea64
Copy link
Contributor

dynamically-generated alias names if/when attribute arguments are allowed to be expressions

Also along these lines, taking an identifier as an attribute argument will break ever being able to use a just variable in that position. Seems like very high cost.

@marcaddeo
Copy link
Contributor Author

(bolding mine) Exactly, and that isn't the case here. The proposed [alias] attribute and identifier attribute argument don't pave the way for new functionality.

If the [alias] attribute took string literal arguments like all other attributes' arguments, it would at least in theory open the door to e.g.

  • non-identifier aliases (for example using ! as alias, being able to invoke a recipe like just !)
  • dynamically-generated alias names if/when attribute arguments are allowed to be expressions

... and the time to add [alias] attribute would be when introducing such new functionality, to maximize flex in tailoring it appropriately.

My first attempt at this was extending StringLiteral to hold a Name, so that the attribute would be [alias("name")] but this ended up being somewhat messy to implement since it required adding new conditions in the error handling to deal with reporting on individual tokens. This can be seen in the first commit in this PR. My implementation may have just been poor, though.

My second attempt I switched to using Name since that's what aliases already are, which made handling them in the code easier and require less refactoring in other places.

However, doesn't this help to pave the way to work with some of those cases you've stated above? By wrapping attribute arguments in AttributeArgument, the parser can assign more than one type as an argument instead of just StringLiteral. Then the parse_attribute method can pass, e.g. Expression or Conditional as an argument to an attribute. Granted, I don't have as good an understanding of the codebase.

This implementation isn't necessary the best one, but ideally I'd like to find the correct one through discussion.

@laniakea64
Copy link
Contributor

laniakea64 commented Sep 6, 2024

However, doesn't this help to pave the way to work with some of those cases you've stated above?

The given example justfile syntax was -

[alias(b)]
bar:
  echo "bar"

Once this is valid syntax, b can never be a just variable without breaking backwards compatibility:

b := 'variableB'

[alias(b)] # since this worked as a literal b before, it must always be a literal b, it cannot change to variableB
bar:
    echo "bar"

Whereas string literal wouldn't close that option:

b := 'variableB'

[alias("b")] # this would stay a literal b even if variables/expressions become allowed here, and does not interfere with later ability to use the variable b in this position
bar:
    echo "bar"

By wrapping attribute arguments in AttributeArgument, the parser can assign more than one type as an argument instead of just StringLiteral. Then the parse_attribute method can pass, e.g. Expression or Conditional as an argument to an attribute. Granted, I don't have as good an understanding of the codebase.

Sounds like these internal changes could help implement #2287 (comment) / #1955 (comment) 👍 If casey agrees that's the way to do it, that part would probably be best as a separate PR tagging those issues and not adding new attributes. Up to casey.

@marcaddeo
Copy link
Contributor Author

marcaddeo commented Sep 6, 2024

However, doesn't this help to pave the way to work with some of those cases you've stated above?

The given example justfile syntax was -

[alias(b)]
bar:
  echo "bar"

Once this is valid syntax, b can never be a just variable without breaking backwards compatibility:

b := 'variableB'

[alias(b)] # since this worked as a literal b before, it must always be a literal b, it cannot change to variableB
bar:
    echo "bar"

Whereas string literal wouldn't close that option:

b := 'variableB'

[alias("b")] # this would stay a literal b even if variables/expressions become allowed here, and does not interfere with later ability to use the variable b in this position
bar:
    echo "bar"

[alias(b)] doesn't conflict with the b variable currently:

± cat just-test
b := "variableB"

[alias(b)]
bar:
  echo "{{ b }}"

± cargo run --bin just -- --justfile just-test b
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.10s
     Running `target/debug/just --justfile just-test b`
echo "variableB"
variableB

EDIT: I may have misunderstood. You’re saying the current output would be problematic because you couldn’t do just variableB if expressions were allowed inside attributes?

EDIT2: However, this is how aliases currently behave. An alias name is a Name. So should the alias attribute allow different behavior, i.e. evaluated alias names?

@casey
Copy link
Owner

casey commented Sep 6, 2024

I think the ideal behavior would be that the [alias] attribute takes a string literal, but that string literal must be a valid name. In the future, it could be relaxed to allow for non-identifier aliases.

I can definitely see the merits of the arguments here, pro and con. I'm slightly leaning towards not adding a new attribute, but if other people want it, then I could see adding it. I think for now I'm leaning towards closing this PR, but I opened #2349 so the feature can be discussed.

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.

4 participants