JoshCheek
commented
8 years ago Thanks for the PR!
Environment Variables
So the environment variables are really interesting. Previously this PR would not have worked, because it would not know how to find Ruby (eg if you started Atom from Spotlight, then it wouldn't inherit your environment, so nothing would add the right dir to the PATH).
They apparently just fixed this by launching a new shell and recording the environment variables, and setting them for Atom so that it doesn't matter how you launch it.
This is a pretty big deal, it means that users don't need to go to all the effort of figuring out how to execute their Ruby without an environment set. The downside, though, is that it removes customizability from our users, eg they can't run SiB under a different environment than their shell. Still, for the end-user simplicity, it's probably worth it (a user sophisticated enough to have these needs is probably also sophisticated enough to just open the package and edit it).
Config
Update to new atom config system
Nice, back when I tried to do this, the variables weren't showing up, but it looks like they fixed it in https://github.com/atom/settings-view/pull/371
Sadly, it still doesn't allow objects, even though docs say that they do. If we remove env vars, then we don't need them, so I guess it's probably fine.
Builtin Errors
Use the atom built-in error messages, instead of alert
Love it!
Verbose Toggle
Add a verbose toggle in config, for displaying console.log()
I like the idea, but don't see the purpose of toggling when it's just going to the console, which users don't see, anyway. The real purpose of the logging is that I have to do a certain amount of debugging (b/c previously it did not inherit the environment), and it does its best to explain the environment that the code is running in. Maybe what I really want is to notify that information if the error implies that Ruby or SiB couldn't be found? That might be nice, b/c it would be apparent to the user, without them having to know enough about Atom to realize they can even open a console.
Path to SiB
The only mandatory config option is the path to SiB (like Rubocop-linter for atom) Remove support for setting ruby path, in favor of setting abs path to SiB
Can you explain the rationale on this one?
Issue #8
Remove support for issue #8
Should be fine since that var was set in his env, which will now be inherited.
Fixing #23
Fix #23
Ahh, I probably missed this issue >.< There is an easy solution for it of just setting this line to sibConfig = atom.config.get('seeing-is-believing') ? @configDefault A pretty easy stopgap.
Readme
Update the readme to reflect the changes
Ty :)
Update to new atom config system Use the atom built-in error messages, instead of alert Add a verbose toggle in config, for displaying console.log() The only mandatory config option is the path to SiB (like Rubocop-linter for atom) Remove support for setting ruby path, in favor of setting abs path to SiB Remove support for issue #8 Fix #23 Update the readme to reflect the changes
I removed the env support because the code confused me, and as such refactoring it was hard, so while this PR is a step backwards in regards to issue #8, it does revive what was essentially a dead atom package (didn't work properly), in its state prior to fixing issue #8.
The new atom config screen