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

Replace realpath occurrences with a custom function #114

Merged
merged 3 commits into from
Feb 5, 2021

Conversation

fcrespo82
Copy link
Contributor

@fcrespo82 fcrespo82 commented Nov 19, 2020

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:

  1. Create another file in another folder with the function to be sourced by the other files;
  2. Create the function in every file that needs it;
  3. 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
Copy link

jlurena commented Nov 29, 2020

can this be merged?

@fcrespo82 fcrespo82 marked this pull request as ready for review November 29, 2020 19:36
@fcrespo82
Copy link
Contributor Author

It is ready for review, and further discussion.

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

Copy link
Contributor

@joschi joschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@fcrespo82
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@joschi
Copy link
Contributor

joschi commented Jan 12, 2021

@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
Copy link
Contributor Author

fcrespo82 commented Jan 12, 2021

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

  1. Create another file in another folder with the function to be sourced by the other files;
  2. Create the function in every file that needs it;
  3. 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
Copy link
Owner

halcyon commented Jan 12, 2021

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

  1. Create another file in another folder with the function to be sourced by the other files;
  2. Create the function in every file that needs it;
  3. 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
Copy link
Contributor Author

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
Copy link
Owner

halcyon commented Feb 5, 2021

Thank you @fcrespo82 . LGTM!

@halcyon halcyon merged commit e5e43b7 into halcyon:master Feb 5, 2021
@fcrespo82 fcrespo82 deleted the replace-realpath branch February 5, 2021 13:12
joschi added a commit to joschi/asdf-java that referenced this pull request Feb 5, 2021
joschi added a commit to joschi/asdf-java that referenced this pull request Feb 8, 2021
halcyon pushed a commit that referenced this pull request Feb 8, 2021
* Remove surplus `dirname` in set-java-home.bash
* Remove surplus `dirname` in set-java-home.zsh
* Remove surplus `dirname` in set-java-home.fish

Refs #114
Closes #126
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.

realpath on mac OS
4 participants