oh-my-fish / plugin-foreign-env

Run foreign bash scripts and capture exported environment variables
MIT License
221 stars 15 forks source link

feat: rewrite and make it more robust #28

Closed stasjok closed 1 year ago

stasjok commented 1 year ago

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

It:

Caveats:

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.

stasjok commented 1 year ago

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 commented 1 year ago

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.

stasjok commented 1 year ago

I've applied your suggestions.

scorphus commented 1 year ago

I wonder why the build doesn't get triggered

scorphus commented 1 year ago

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