-
Notifications
You must be signed in to change notification settings - Fork 785
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
feat: unset all enviroment variables ASDF_{PLUGIN}_VERSION #1632
base: master
Are you sure you want to change the base?
Conversation
it is a feature create by recommendation of issue 1619
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, have a few comments to maximize compatibility with shells and the rest of the codebase.
line indetation, elvish custom print and add parameter -v in unset_all
LGTM - CI maybe needs to be ran by a maintainer though (since #1624 was merged). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding new flags we would like to get some test cases for both implemented code paths, even if just the happy paths.
Looks good otherwise 💯
Great. I will try to create some tests, from what I had seen, there are no tests related to the Shell command yet, i will try to cover my feature and previous features =]. |
@hyperupcall and/or @jthegedus, how can i source asdf file? I suppose to source to integrate with shell. |
@edvardsanta Information about shell initialization on our documentation website. It could be that you are getting that error because you forgot to run asdf's initialization before running your tests. If you are getting errors from only that particular test, look at other tests to see how it's done. Please also look at the asdf code that prints that error: |
@edvardsanta I did already review and approve your code - now you only need to update your code as per jthegedus's feedback. |
This specific test that i am creating is not working, but i will read the doc |
Feel free to commit the WIP tests and get the CI to execute them. The tests need some work to separate out some of the shell-specific tests which might be difficult to run locally. |
i expect it works =] - i had to copy asdf.sh to source it
remove my older file remove function created about shell in test helpers
for some reason i changed to 'unstall''
I think i finalized the feature, let me know if i might change something [=. |
@jthegedus can you check my pr? pls |
@jthegedus, any updates? |
@edvardsanta Have you seen the PR backlog? I'm sure the maintainers are aware of it and will get to it (including this PR) when they can get to it. Sending multiple pings isn't likely going to do anything, and if it were me you were pinging, it would actually encourage me to ignore the PR. |
Well, this has been active for a few months now, it's easier and better for me to send a message and ask than to guess, since my last message was a few months ago. Feel free to recommend ignoring this pr. |
it is a feature create by recommendation of issue 1619
Summary
It unset all enviroment variables ASDF_{PLUGIN}_VERSION
Closes: #1619
Other Information
I was not able to test in elvish. Because of that it just prints a description