ioquatix / script-runner

This package will run various script files inside of Atom. It currently supports JavaScript, CoffeeScript, Ruby, and Python. You can add more.
http://atom.io/packages/script-runner
Other
62 stars 23 forks source link

Possibly refactor this line? #11

Closed ioquatix closed 7 years ago

ioquatix commented 9 years ago

https://github.com/ioquatix/script-runner/blob/b191f0292bfead5a0658b3e13e67da0f0aa2e8be/lib/script-runner.coffee#L52

Is the only case where this is necessary later on in https://github.com/ioquatix/script-runner/blob/b191f0292bfead5a0658b3e13e67da0f0aa2e8be/lib/script-runner.coffee#L78-L81

.. because sometimes I think that if there is only one case for this behaviour, perhaps it's best it should be directly where it is required. Otherwise, it might hide some other bugs unexpectedly. What do you think @huba

huba commented 9 years ago

There is another case of the behavior in killProcess at https://github.com/ioquatix/script-runner/blob/b191f0292bfead5a0658b3e13e67da0f0aa2e8be/lib/script-runner.coffee#L114-L116

A specific process needs to be stopped when the stop key-binding is fired or when the pane destroys the runner tab associated with the process.

ioquatix commented 9 years ago

If this is a common pattern, what about a function, e.g. killProcessByPane which does the lookup, the null check, and then calls killProcess if appropriate?

huba commented 9 years ago

Why not make use of the signal emitted by the pane when it destroys a tab? https://atom.io/docs/api/v0.174.0/Pane#instance-onWillDestroyItem

huba commented 9 years ago

I think what we might need is to make a better structure to keep track of running scripts.

ioquatix commented 9 years ago

@huba yeah I'm fine with that, if you want to propose something. However, if it makes the code significantly more complex for little benefit, that isn't very useful.

One thing I've been thinking about is the ability to restart scripts after you quit and reload atom. It would be nice if we could have a restart button/command which re-runs the script. A data structure to contain all that information might be appropriate, some kind of model to back the current view/pane.

huba commented 9 years ago

https://github.com/huba/script-runner/commit/568a2b06ef9cf2104a949b1f5aa7ce445b56cca9

@ioquatix what do you think? It's nowhere near ready for a PR. It's aiming to reduce code complexity by avoiding duplication of functionality provided by atom's API. I'm also trying to make interactions and manipulation happen at the model layer.

We will have to add atom-space-pen-views as a dependency because they will remove ScrollView from the core later on.

ioquatix commented 9 years ago

@huba that looks really good in general. Perhaps do a bit more testing and let me know if you'd like me to try it out too.

ioquatix commented 7 years ago

I think this is a good idea, is this still relevant and can we apply it?

ioquatix commented 7 years ago

We've revamped this part of the plugin and it now uses panels.