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

feat: unset all enviroment variables ASDF_{PLUGIN}_VERSION #1632

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

edvardsanta
Copy link
Contributor

@edvardsanta edvardsanta commented Sep 6, 2023

it is a feature create by recommendation of issue 1619

Summary

It unset all enviroment variables ASDF_{PLUGIN}_VERSION
image
fish

Closes: #1619

Other Information

I was not able to test in elvish. Because of that it just prints a description

it is a feature create by recommendation of issue 1619
@edvardsanta edvardsanta requested a review from a team as a code owner September 6, 2023 19:46
@edvardsanta edvardsanta changed the title feat: unset all enviroment variavles feat: unset all enviroment variables ASDF_{PLUGIN}_VERSION Sep 6, 2023
Copy link
Contributor

@hyperupcall hyperupcall left a 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.

lib/commands/command-export-shell-version.bash Outdated Show resolved Hide resolved
lib/commands/command-export-shell-version.bash Outdated Show resolved Hide resolved
lib/commands/command-export-shell-version.bash Outdated Show resolved Hide resolved
edvardsanta and others added 3 commits September 10, 2023 22:29
line indetation, elvish custom print and add parameter -v in unset_all
@hyperupcall
Copy link
Contributor

hyperupcall commented Sep 12, 2023

LGTM - CI maybe needs to be ran by a maintainer though (since #1624 was merged).

Copy link
Contributor

@jthegedus jthegedus left a 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 💯

@edvardsanta
Copy link
Contributor Author

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 =].

@edvardsanta
Copy link
Contributor Author

@hyperupcall and/or @jthegedus, how can i source asdf file? I suppose to source to integrate with shell.
image

@hyperupcall
Copy link
Contributor

hyperupcall commented Sep 15, 2023

@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: Shell integration is not enabled.... Those are just some ideas to figure out what your issue is - I haven't ran across that myself.

@hyperupcall
Copy link
Contributor

@edvardsanta I did already review and approve your code - now you only need to update your code as per jthegedus's feedback.

@edvardsanta
Copy link
Contributor Author

@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: Shell integration is not enabled.... Those are just some ideas to figure out what your issue is - I haven't ran across that myself.

This specific test that i am creating is not working, but i will read the doc

@jthegedus
Copy link
Contributor

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.

@edvardsanta
Copy link
Contributor Author

edvardsanta commented Sep 21, 2023

image
same problem here, i might see where is this log and do some changes again

Edit: I created the tests, sorry for too many commits, I thought there was no test for the shell command, but I found it, so I just created my test there

@edvardsanta
Copy link
Contributor Author

I think i finalized the feature, let me know if i might change something [=.

@edvardsanta
Copy link
Contributor Author

@jthegedus can you check my pr? pls

@edvardsanta
Copy link
Contributor Author

@jthegedus, any updates?

@hyperupcall
Copy link
Contributor

@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.

@edvardsanta
Copy link
Contributor Author

@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.

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.

asdf shell --unset to unset all plugin shell versions
3 participants