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.

Sign up for GitHub

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

Jump to bottom

Use new syntax in rbenv module #614

Merged
merged 1 commit into from
Sep 22, 2023
Merged

Use new syntax in rbenv module #614

merged 1 commit into from
Sep 22, 2023

Conversation

marcelarie
Copy link
Contributor

@marcelarie marcelarie commented Sep 21, 2023 •
edited
Loading

Related to this issue: #613 (comment)

@fdncred
Copy link
Collaborator

fdncred commented Sep 21, 2023 •
edited
Loading

I'm not sure how this works if you use the module, since nothing is being exported, but I'm no module expert. @amtoine, when you get a sec, can you check this?

amtoine
amtoine reviewed Sep 21, 2023
View reviewed changes
modules/rbenv/rbenv.nu Outdated Show resolved Hide resolved
@amtoine amtoine linked an issue Sep 21, 2023 that may be closed by this pull request
Module rbenv can't export command named same as module. #613
Closed
amtoine
amtoine previously requested changes Sep 21, 2023
View reviewed changes
Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

@marcelarie

i think you should just rename the rbenv command to main, without changing the export nor the def-env and revert the other changes 😉

@marcelarie
Copy link
Contributor Author

True @amtoine , idk why I did not try that before 🤕

@amtoine amtoine dismissed their stale review September 21, 2023 17:42

requested changes have been implemented

amtoine
amtoine approved these changes Sep 21, 2023
View reviewed changes
Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

True @amtoine , idk why I did not try that before 🤕

ehe 😉

now it looks good to me, could you try it to make sure use rbenv.nu works as expected?

@marcelarie
Copy link
Contributor Author

marcelarie commented Sep 22, 2023 •
edited
Loading

now it looks good to me, could you try it to make sure use rbenv.nu works as expected?

Screen.Recording.2023-09-22.at.12.44.38.mov

Works correctly yes!

@marcelarie
Copy link
Contributor Author

@amtoine I just checked and it loads the versions correctly but the output of some commands don't work.

for example rbenv shows the help menu.
but rbenv version does not return anything.
rbenv commands does not return anything.

I will check what happens.

@marcelarie
Copy link
Contributor Author

it arrives correctly to line 26 with rbenv version but it does not show the output of the command.

@amtoine
Copy link
Member

amtoine commented Sep 22, 2023

looks like an issue for another PR maybe?

@amtoine amtoine merged commit c3086bc into nushell:main Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amtoine amtoine amtoine approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Module rbenv can't export command named same as module.
3 participants
@marcelarie @fdncred @amtoine