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

alias with arguments does not take additional arguments into consideration #605

Closed
maxandersen opened this issue Dec 16, 2020 · 14 comments · Fixed by #1753
Closed

alias with arguments does not take additional arguments into consideration #605

maxandersen opened this issue Dec 16, 2020 · 14 comments · Fixed by #1753

Comments

@maxandersen
Copy link
Collaborator

when doing jbang alias add something.java -S , i.e. have the alias seeded with some arguments
then doing jbang something works, but jbang something additionalarg fails.

we are somehow ignoring/combining arguments in a non-working fashion when using aliases.

@maxandersen
Copy link
Collaborator Author

the concrete example I had was an alias like jbang alias add -f . --name serve com.intuit.karate:karate-core:0.9.9.RC2 -S

in this case you should be able to do jbang serve -p 8000 and see it start on another port.

But it doesn't - it just runs as if no arguments was passed.

@maxandersen
Copy link
Collaborator Author

@quintesse any idea ?

@quintesse
Copy link
Contributor

@maxandersen yes, that's how I designed it :-)

Which might be wrong of course. But it follows the same idea as //REPOS, there are default arguments/repos, when you manually specify arguments/repos the default ones are overridden.

I actually thought about allowing some kind of syntax/option that says that additional arguments should be added instead of overriding the default ones but it's not something that's currently implemented.

@maxandersen
Copy link
Collaborator Author

Well its broken no matter what as the Alias call without argument works but with argument it fails :)

I think the usecase above is a great usecase where Aliases should allow appending of arguments.

@quintesse
Copy link
Contributor

Ah so if you run jbang serve com.intuit.karate:karate-core:0.9.9.RC2 -S it doesn't work?

@maxandersen
Copy link
Collaborator Author

Here is the issue:

jbang alias add -f . --name run com.intuit.karate:karate-core:0.9.9.RC2 -S

now, jbang run works and runs karate with -S

but doing jbang run -p 8000 it runs as if only -p 8080 have been passed.

afaik both alias in bash/zsh/git the arguments are part of the alias, so anything additional are simply appended.

@quintesse
Copy link
Contributor

quintesse commented Dec 19, 2020

Ok, but that means it does work as I at least intended, it's not broken, you just don't like how it works :-)

@quintesse
Copy link
Contributor

I have no problem with changing it to the way you want btw. I considered both options at the time and just chose one.

@maxandersen
Copy link
Collaborator Author

Ok, but that means it does work as I at least intended, it's not broken, you just don't like how it works :-)

True :) I didn't realise this specific app ignored irrelevant flags:)

@maxandersen
Copy link
Collaborator Author

I have no problem with changing it to the way you want btw. I considered both options at the time and just chose one.

I think appending actually gives value as it means you can simplify and compose.

Where as with replacement you don't really get much help.

@quintesse
Copy link
Contributor

My way of thinking was that in most cases the most difficult thing about an alias is the URL, hard to remember and hard to type. At the same time once you specify arguments there is often no way to "unspecify" them, so overriding them instead of appending them seemed useful. But I admit the choice was a somewhat arbitrary and I have no issue with doing it your way.

@robin-a-meade
Copy link

The current behavior surprised me.

I creating an alias named color_sqlline:

jbang alias add 
  --name=color_sqlline \
  --deps com.oracle.database.jdbc:ojdbc8:23.3.0.23.09,com.h2database:h2:2.0.204 \
  sqlline:sqlline:1.12.0 --color=true

jbang color_sqlline ⇐ Will use color
jbang color_sqlline --nullValue='***' ⇐ Will NOT use color

I'm used to bash aliases. Consider:

alias ls='ls --color=auto'

ls ⇐ Will use color
ls -l ⇐ Will STILL use color

quintesse added a commit to quintesse/jbang that referenced this issue Feb 21, 2024
Arguments passed to an alias are now appended to any arguments that are
already defined in the alias instead of replacing them.

Fixes jbangdev#605
@quintesse
Copy link
Contributor

@maxandersen I've created a PR for this, but it's somewhat of a breaking change of course, so you need to decide if you want this or not :-)

quintesse added a commit to quintesse/jbang that referenced this issue Feb 21, 2024
Arguments passed to an alias are now appended to any arguments that are
already defined in the alias instead of replacing them.

Fixes jbangdev#605
quintesse added a commit to quintesse/jbang that referenced this issue Feb 22, 2024
Arguments passed to an alias are now appended to any arguments that are
already defined in the alias instead of replacing them.

Fixes jbangdev#605
quintesse added a commit to quintesse/jbang that referenced this issue Feb 22, 2024
Arguments passed to an alias are now appended to any arguments that are
already defined in the alias instead of replacing them.

Fixes jbangdev#605
quintesse added a commit to quintesse/jbang that referenced this issue Feb 22, 2024
Arguments passed to an alias are now appended to any arguments that are
already defined in the alias instead of replacing them.

Fixes jbangdev#605
maxandersen pushed a commit that referenced this issue Feb 22, 2024
Arguments passed to an alias are now appended to any arguments that are
already defined in the alias instead of replacing them.

Fixes #605
@maxandersen
Copy link
Collaborator Author

@robin-a-meade thanks for reminding us to fix this :)

@quintesse thanks merged! and yes its a breaking change but a good one imo.

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 a pull request may close this issue.

3 participants