search

bcomnes / sublime-standard-format

:sparkles: Runs standard --fix against the javascript in your ST3 window on save or manually.
https://packagecontrol.io/packages/StandardFormat
MIT License
60 stars 21 forks source link

Fix #63, PATH issues on Linux #64

Closed adnanademovic closed 7 years ago

adnanademovic commented 7 years ago

The first problem was the fact that the first line on /usr/bin/env bash -i -c -l "echo $PATH" is the PID of the isolated process, so I've made it ignore it.

The second problem was the fact that the global environment was overwritten, which could have been bad for other reasons. I've made the Popen commands use a custom crafted environment. The performance overhead is tiny, since the only copying done are copies to references.

bcomnes commented 7 years ago

Sweet thank you! Looks good. I just woke up, but I'll pull this down, give it a deeper read and try to land a release today.

adnanademovic commented 7 years ago

Any updates on the situation?

I'm currently just using my fork, so don't feel like I'm trying to rush you.

bcomnes commented 7 years ago

Sorry, just been super swamped. I will get this done this coming week, shooting for monday/tuesdayish. Testing involved since I try to do it on at least mac os and windows.

bcomnes commented 7 years ago

Ok looking at this now.

bcomnes commented 7 years ago

I like the changes. Thank you for your work!

They initially broke on my mac due to the changes to the global path searching. I made a few changes on the https://github.com/bcomnes/sublime-standard-format/tree/bret-edit branch which basically looks through all lines of shell output for lines beginning with os.sep and then grabs the first one it finds. If none are found then the default ENV that sublime text comes up with is used instead... although this is painfully sparse and doesn't include any changes to the PATH that people set in their interactive .bashrc init scripts so commonly.

The local env changes look good as well. Thank you for noticing and fixing that.

I would like to merge my branch (which includes your branch). Do you mind giving that a whirl or review?

I still need to test on windows.

adnanademovic commented 7 years ago

Change looks good - really clever solution 👍

Just to give you input: my fork works on Windows 10, and it's not affected by your change - so on my end, Windows works fine.

bcomnes commented 7 years ago

This code path has always been the weakest part of the environment discovery process and I'm glad to to take it a few steps forward. Sublime should really provide this type of functionality for plugins imo. Thanks for the review and the contribution! I'll cut a release and give you a shoutout in the notes.

bcomnes commented 7 years ago

Merged and released as 6.1.1. Should be in package control in a few hours.