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

Update to work with the latest version of atom #24

Merged
merged 1 commit into from
Apr 22, 2016
Merged

Update to work with the latest version of atom #24

merged 1 commit into from
Apr 22, 2016

Conversation

martoko
Copy link
Contributor

@martoko martoko commented Apr 16, 2016

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
screenshot from 2016-04-16 12-39-31

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
@JoshCheek
Copy link
Owner

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 atom/settings-view#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 :)


mainCommand = atom.config.get('seeing-is-believing.sibCommand')
args = atom.config.get('seeing-is-believing.flags')
args.concat(additionalArgs) if additionalArgs?
Copy link
Owner

Choose a reason for hiding this comment

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

Concat here doesn't modify the array, it returns a new one, so the additional args won't work (thus annotate-magic-comments and remove-annotations don't work)

@JoshCheek
Copy link
Owner

Nvm, I got it. This is nice, I've got it pulled locally, refactored it a bit, removed a lot of stuff that we don't need anymore. I'll release it when I get home (getting kicked out of this place >.<)

@JoshCheek JoshCheek merged commit 58a4f4f into JoshCheek:master Apr 22, 2016
@JoshCheek JoshCheek mentioned this pull request Apr 22, 2016
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.

Uncaught TypeError: Cannot read property 'add-to-env' of undefined
2 participants