Replace realpath occurrences with a custom function

halcyon / asdf-java

A Java plugin for asdf-vm.

MIT License

455 stars 86 forks source link

Replace realpath occurrences with a custom function #114

Closed fcrespo82 closed 3 years ago

fcrespo82 commented 3 years ago

This pull request is aimed to fix #109

I only updated the code on the functions file, we should change in the set-java-home.* files too. I just need to get your feedback if I should:

Create another file in another folder with the function to be sourced by the other files;
Create the function in every file that needs it;
Source the functions file on the other files where the function is needed.

My personal preference would be for the first option, but I leave it here for discussion.

jlurena commented 3 years ago

can this be merged?

fcrespo82 commented 3 years ago

It is ready for review, and further discussion.

If the maintainers approve, yes, it can be merged.

fcrespo82 commented 3 years ago

@joschi Just wait a sec... do not merge yet. I forgot that we need to address if we should change in the set-java-home.* files too.

Look on the original PR text...

fcrespo82 commented 3 years ago

Pinging @joschi and @halcyon. Which option should I follow?

joschi commented 3 years ago

@fcrespo82 Sorry for the late response!

I think it makes sense to update the set-java-home.* scripts too.

@halcyon What do you think?

fcrespo82 commented 3 years ago

@joschi @halcyon Yes, I think it makes sense too, but should I?

Create another file in another folder with the function to be sourced by the other files;

Create the function in every file that needs it;

Source the functions file on the other files where the function is needed.

My personal preference would be for the first option, but I leave it here for discussion.

halcyon commented 3 years ago

@joschi @halcyon Yes, I think it makes sense too, but should I?

Create another file in another folder with the function to be sourced by the other files;

Create the function in every file that needs it;

Source the functions file on the other files where the function is needed.

My personal preference would be for the first option, but I leave it here for discussion.

I'm leaning towards options 2 and 3.

Option 3 seems the most sensible, but I can see how it could potentially result in performance issues on some platforms (like WSL2) that are performance sensitive (due to the defining of a bunch of functions that are never used).
Option 2 has the lowest runtime overhead, but increases maintenance cost.
Option 1 looks like it could be a good compromise but may add its own new issues around complexity and setting a precedent for adding new files when we add new functions.

fcrespo82 commented 3 years ago

Hi folks, I went with the approach number 2 to get this out of the way.

If you find this should be changed, please let me know.

Thanks!

halcyon commented 3 years ago

Thank you @fcrespo82 . LGTM!