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: rewrite and make it more robust #28

Merged
merged 2 commits into from
Jan 14, 2023
Merged

Conversation

stasjok
Copy link
Contributor

@stasjok stasjok commented Dec 30, 2022

I know it's a radical change, but I rewrote it. The main reason is speed.

It:

  • doesn't use any external commands like sed/tr
  • faster (no external commands, one loop)
  • should be more robust, even handles newlines in variables
  • doesn't need to override standard descriptors (stdin/stdout/stderr), probably should fix Command asking input during execution #14 (not tested), stderr is returned to stderr

Caveats:

  • supports only fish 3.0+, because it doesn't split explicitly PATH-like variables by colon, fish does it itself, but only 3.0+
  • propagates correct exit code only in fish 3.1+, because it uses $pipestatus, fish 3.0 always return 0, but still works
  • env should support -0, --null argument, it added to GNU coreutils long time ago, but I don't know about OS other than Linux
  • it uses a hard-coded descriptor 31, so if bash commands use it then there would be errors

It is less portable/compatible, so I don't think someone will merge it, but I created a pull request in case someone would want to try it.

Copy link
Member

@scorphus scorphus left a comment

Choose a reason for hiding this comment

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

Hey, I haven't read the patch mostly because it changes indentation, and that makes it harder to tell what really changed.

So, to start with, could you please keep the indentation as 2 spaces?

@stasjok
Copy link
Contributor Author

stasjok commented Dec 31, 2022

Hey, I haven't read the patch mostly because it changes indentation, and that makes it harder to tell what really changed.

So, to start with, could you please keep the indentation as 2 spaces?

I'm using official formatter (fish_indent). It doesn't allow to change indent level, so I think 4 spaces is official. I recommend using it too.

Anyway, I tried to undo any changes fish_indent done. But the only file that wasn't changed entirely is test/bootstrap.fish. I added to it running tests in separate instances, because earlier all global variables was shared between tests. Also I prepended functions dir to fish_function_path so that I can test without installing it (should also be useful in CI, doesn't need to call omf).

@scorphus
Copy link
Member

scorphus commented Jan 3, 2023

Thanks for reducing the size of the diff, @stasjok. Really appreciated 😊

I'm using official formatter (fish_indent). It doesn't allow to change indent level, so I think 4 spaces is official. I recommend using it too.

Absolutely, we can definitely address that in a following PR.

I think the gains in speed and robustness (and simplicity too, I'd argue) outweigh the caveats — more so considering only one official plugin depends on foreign-env. It may be hard to tell if there's any other package out there using foreign-env that may be affected by the changes but even if there is, that shouldn't prevent foreign-env from improving/evolving.

functions/fenv.fish Outdated Show resolved Hide resolved
functions/fenv.main.fish Outdated Show resolved Hide resolved
functions/fenv.fish Outdated Show resolved Hide resolved
@stasjok
Copy link
Contributor Author

stasjok commented Jan 4, 2023

I've applied your suggestions.

@scorphus
Copy link
Member

scorphus commented Jan 4, 2023

I wonder why the build doesn't get triggered

@scorphus scorphus force-pushed the rework branch 2 times, most recently from f00deff to 8ee775e Compare January 5, 2023 21:23
@scorphus
Copy link
Member

scorphus commented Jan 5, 2023

I guess we're good to go. I've just confirmed that plugin-nvm works okay.

Copy link
Member

@scorphus scorphus left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, @stasjok. And also thanks for things I learned from you here 👍

Sorry for the delay.

@scorphus scorphus merged commit 3ee9553 into oh-my-fish:master Jan 14, 2023
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.

Command asking input during execution
2 participants