Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Add fix-path option. #35

Closed
wants to merge 1 commit into from

Conversation

lexicalunit
Copy link

On OS X electron frequently does not seem to pick up the PATH setting
correctly when launching Atom from a GUI, for example the Dock. This
means that shellcheck can not be found. One workaround is to specify the
absolute path to shellcheck, but this can be done automatically. This
provides a much better experience for users on OS X.

This should also resolve issue #7 for many users.

  • Adds enableFixPath option.
  • Adds documentation for option to README.
  • Adds observer for option, once turned on it can't be reversed.
  • Defaults enableFixPath to true for 'darwin` (OS X) platform.
  • Provides user documentation for option, indicating its use in OS X.
  • Adds fix-path dependency.

@@ -12,6 +12,9 @@
"dependencies": {
"atom-linter": "^3.0.0"
},
"optionalDependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

This should be a regular dependency as it will fail if it can't be found.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, after thinking about it a bit more you're totally right here. Fixing...

@Arcanemagus
Copy link
Member

Are there any downsides to enabling this unconditionally?

@lexicalunit
Copy link
Author

I don't think there are any downsides, but I want to make sure not to break things somehow for existing users, and especially not for non-OS X users. So default off makes more sense to me, at least for the next few releases. That was also my initial thinking on using optionalDependency.

On OS X electron frequently does not seem to pick up the PATH setting
correctly when launching Atom from a GUI, for example the Dock. This
means that shellcheck can not be found. One workaround is to specify the
absolute path to shellcheck, but this can be done automatically. This
provides a much better experience for users on OS X.

This should also resolve issue AtomLinter#7 for many users.

- Adds `enableFixPath` option.
- Adds documentation for option to README.
- Adds observer for option, once turned on it can't be reversed.
- Defaults `enableFixPath` to true for 'darwin` (OS X) platform.
- Provides user documentation for option, indicating its use in OS X.
- Adds `fix-path` dependency.
@Arcanemagus
Copy link
Member

If you check the fix-path source it does nothing if the environment is not darwin 😉.

@Arcanemagus
Copy link
Member

@kepler0 I know you use OS X, thoughts on this?

@lexicalunit
Copy link
Author

Haha well then! In that case yeah, I've had lots of issues with PATH on OS X with Atom, even Atom Beta which I am currently using. I'd love to have fix-path included by default in all the linters I use. At least until the issue has been definitely resolved in either Electron or Atom itself.

@@ -10,7 +10,8 @@
"atom": ">0.180.0"
},
"dependencies": {
"atom-linter": "^3.0.0"
"atom-linter": "^3.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda offtopic but we might wanna bump this version too

@keplersj
Copy link

keplersj commented Dec 9, 2015

Oh shit. Forgot about this notification, @Arcanemagus. Um, I don't have an issue with this solution per say, but since this effects more use cases than shellcheck I'd perfer this solution in a broader scope.

Ideally I'd like this to solved within Atom itself, however this issue doesn't seem to be on the roadmap of either Atom or Electron. @joefitzgerald made a pretty good solution in the form of a package, with a service-based API which can be interacted with by other packages; however telling every OS X user who wishes to open Atom as an Application Bundle (from Launchpad, Open with Atom in Finder, the Dock, or from Finder itself) to install yet another package on top of linter and their linter provider(s), to solve a shortcoming in Atom, is far from ideal.

I support linter providers implementing a solution like this, but with hesitation because the entire situation is far from ideal.

@keplersj
Copy link

keplersj commented Dec 9, 2015

It's also worth noting now that I've reread my comment that this really is a huge oversight of Atom/Electron. Applications built for OS X are meant to be launched as Application Bundles, not command line programs. Building an application for OS X which sports the ability to interact with command line programs, like any Electron app due to it's implementing Node, should be held responsible for inheriting the user's environment where command line options are configured in the first place. Every other application on OS X which sports command line integration handles this; iTerm 2, Xcode, IntelliJ, Eclipse, every Java applet (which means Minecraft can handle want Atom currently can't), Visual Studio Code (an Electron app), MonoDevelop, NeoVim, and Black Screen (yet another Electron app).

This is absolutely ridiculous at this point. linter and it's providers should not have to implement what should already be in the text editor.

@Arcanemagus
Copy link
Member

Absolutely agree with you, this is something that should be handled within Atom as it causes nothing but problems since general users are not going to understand what is wrong.

@Arcanemagus
Copy link
Member

Since the place where this would be worked around in most linter packages is atom-linter, we might as well continue the discussion here: steelbrain/atom-linter#62

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants