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

Fix fish shell (de)activate #460

Merged
merged 6 commits into from
Sep 18, 2023
Merged

Fix fish shell (de)activate #460

merged 6 commits into from
Sep 18, 2023

Conversation

x0id
Copy link
Contributor

@x0id x0id commented Sep 17, 2023 •
edited
Loading

Description

The fish shell kerl activate/deactivate code has been improved and refactored to fix environment setup/cleanup issues (#458).

The test suite for fish shell based activate/deactivate has been added.

This PR partially (for the fish shell) resolves issue #458.

@paulo-ferraz-oliveira paulo-ferraz-oliveira changed the title The fish shell (de)activate code fixes and test suite Fix fish shell (de)activate Sep 17, 2023
@paulo-ferraz-oliveira
Copy link
Contributor

👍 very nice, @x0id, thanks.

I don't know enough fish to do a proper review on this, but it seems solid, especially since it's tested (if I understood correctly, similar to its bash counterpart).

@jadeallenx, I'm Ok to merge as is and then update/fix if there's complaints from the community. What do you say?

@paulo-ferraz-oliveira
Copy link
Contributor

And, @x0id, if you think you'll get around to the rest of #458 soonish, we can probably postpone a release until then, otherwise we can release a patch now and patch later. Thoughts?

@x0id
Use local (function) context for funcstion vars
b37add2 
This does not impact the result of the current tests,
but may help to avoid vars clashes when extending tests.
@x0id
Copy link
Contributor Author

x0id commented Sep 17, 2023

@paulo-ferraz-oliveira, I think there is no rush in releasing the fish shell updates, I'm not sure actually how many people use it. I plan to work on sh/csh in the next couple of weeks.

@jadeallenx
Copy link
Collaborator

@jadeallenx, I'm Ok to merge as is and then update/fix if there's complaints from the community. What do you say?

I think we should merge it too.

@jadeallenx
Copy link
Collaborator

I'm not sure actually how many people use it. I plan to work on sh/csh in the next couple of weeks.

Nothing makes those hidden users show up than a new release that breaks something lol 😄

@paulo-ferraz-oliveira paulo-ferraz-oliveira merged commit fd08a44 into kerl:master Sep 18, 2023
@paulo-ferraz-oliveira
Copy link
Contributor

It's squash-merged. Thanks all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jadeallenx jadeallenx jadeallenx 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.
3 participants
@x0id @paulo-ferraz-oliveira @jadeallenx