Solving many bugs when using artisan #28

Open
wants to merge 2 commits into
from

2 participants

@bvangraafeiland

Previously, many artisan commands didn't work well (try routes, list, or migrate:make), due to incorrect use of the Popen class. These have been fixed. Changes of #25 are included in this pull request as well.

Output from artisan commands is now always shown in a panel, and no distinction between successful execution or failure is made; in either case, the response from artisan is displayed in the panel.

@gnarula
Owner

Hey! Thanks for the pull request. I got my macbook back working today so I can continue with the development of the plugin now. I regret for the inactivity during the last few weeks. Reviewing this ASAP.

EDIT: is the commit checked against Windows? I recall users having issues with it. I'll take a while to get working on a Win environment.

Thanks once again

Gaurav

@bvangraafeiland

Uh, actually it's only been checked against Windows.

@bvangraafeiland bvangraafeiland commented on the diff Jun 20, 2013
generate.py
self.proc_status(proc)
except IOError:
sublime.status_message('IOError - command aborted')
def proc_status(self, proc):
- if proc.poll() is None:
- sublime.set_timeout(lambda: self.proc_status(proc), 200)
@bvangraafeiland
bvangraafeiland added a line comment Jun 20, 2013

This was causing a deadlock if there was a sufficient amount of console output. See also http://docs.python.org/2/library/subprocess.html#subprocess.Popen.wait

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bvangraafeiland bvangraafeiland commented on the diff Jun 20, 2013
generate.py
@@ -79,13 +79,13 @@ def run(self, *args, **kwargs):
def call_artisan(self, command):
try:
self.PROJECT_PATH = self.window.folders()[0]
- self.args = '%s %s %s' % (self.php_path, os.path.join(self.PROJECT_PATH, 'artisan'), command)
+ self.args = [self.php_path, os.path.join(self.PROJECT_PATH, 'artisan')] + command.split(' ')
@bvangraafeiland
bvangraafeiland added a line comment Jun 20, 2013

Popen arguments should be passed in as list rather than string:

If args is a string, the interpretation is platform-dependent and described below. See the shell and executable arguments for additional differences from the default behavior. Unless otherwise stated, it is recommended to pass args as a sequence.

@gnarula
Owner
gnarula added a line comment Jun 20, 2013

I had it as a list before but ran into some issues with the slashes iirc.

@bvangraafeiland
bvangraafeiland added a line comment Jun 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bvangraafeiland

Well to be honest currently it's not that smooth either, ST freezes completely while executing the commands, which can be really noticeable when doing things which take a bit longer, such as migrations. It would be better to execute them in another thread, although some refactoring will probably be needed for that. See also this one: https://github.com/francodacosta/composer-sublime. In there, the output is just printed real time, while not blocking the entire program. I'm still very much a newbie at ST plugins and Python for that matter, I guess.

@gnarula
Owner

After applying this patch or even in the last commit? Earlier sublime.set_timeout() was being used to prevent the plugin from blocking the main thread and it worked well iirc. Yes, we'll have to refactor some code undoubtedly to implement threading.

@bvangraafeiland
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment