halcyon / asdf-java

A Java plugin for asdf-vm.
MIT License
455 stars 86 forks source link

JAVA_HOME on Xonsh shell #113

Closed Kaylebor closed 3 years ago

Kaylebor commented 3 years ago

Hello everyone,

I've been using Xonsh shell for a while, in combination with asdf, and I've found myself with an issue related with JAVA_HOME.

Since I've used this plugin in the past, I checked for the set-java-home.bash script, which didn't work. So, I've created a set-java-home.xsh script, which fixed the issue for me, using Xonsh events (on_chdir and on_post_init).

In short: how would I go about adding that script to the repo? It's the first time I've tried to contribute to an open source project, so I'm slightly lost on that. Git is not an issue, I've already got a separate branch ready.

delgurth commented 3 years ago

@Kaylebor since you are way more knowledgable around xonsh then I'm. Do you have an idea how I can make the xonsh tests in https://github.com/halcyon/asdf-java/pull/142 fail? Adding an exit _.rtn after https://github.com/halcyon/asdf-java/pull/142/files#diff-1db27d93186e46d3b441ece35801b244db8ee144ff1405ca27a163bfe878957fR76 and https://github.com/halcyon/asdf-java/pull/142/files#diff-1db27d93186e46d3b441ece35801b244db8ee144ff1405ca27a163bfe878957fR127 didn't do anything, so I removed that again. And with the other shells, if the grep command fails, the test fails.

Kaylebor commented 3 years ago

Ey @delgurth

Looking around a bit, I believe there are two ways:

raise is the recommended choice (internally it calls to sys.exit); grep -q means to not print anything; err>out is equivalent to 2>&1 in bash (which is also available in xonsh).

Note that sys.exit requires sys to be imported, while raise doesn't.

I've tested it a bit with a local script, and it returned the proper error code expected from grep to the shell, both from bash and xonsh.

I've suggested the appropriate edit; tell me if it works (or doesn't)

delgurth commented 3 years ago

@Kaylebor Thanks a lot, also for the explanation of your suggestion. With the help of the raise SystemExit I found the proper setting which does what I need it to do:

$RAISE_SUBPROC_ERROR = True

I coudn't get your exact code to run, at least not with the xonsh version which was installed in the https://github.com/nektos/act container. But the $RAISE_SUBPROC_ERROR = True is IMHO the better solution, since that way you no longer have to check error status of any command.

I like your addition of the -q option. Why output something you are not interested in.


@halcyon I think this issue can be closed.

Could you please check #136 (simple but nice fix) and my pull requests. Merging all 4 of my pull requests will most likely trigger merge-conflicts because I changed the same file a few times. You won't need to fix those, I'll do that.

Kaylebor commented 3 years ago

Glad to be of help, I prefer your solution too (specially since it works 😅).

Plus, with $RAISE_SUBPROC_ERROR it behaves closer to other shells, which makes the code easier to follow. That's always nice.