pohmelie / run-in-terminal

Atom package for run some commands in terminal, or just run terminal
MIT License
17 stars 7 forks source link

Mac osx terminal support #17

Closed enishii closed 7 years ago

enishii commented 7 years ago

I have added support for the standard Mac OSX terminal. Currently run-in-terminal is able to open the terminal, but not run the code at the same time as well. The workaround to chmod +x the file and add a shebang to the file works, but this is not the intention setup of the program.

The code now checks if the operating system is osx (darwin), and sets up the correct Applescript command to open the terminal and run the code, or open the terminal at the working directory. This only works with the standard terminal in OSX, other terminal apps like iTerm are not supported.

Adding terminal arguments for Mac is not possible. You can however write command commands in the argument (separated by semicolons) field, which will be executed before the script, e.g. "cd /some/dir;". This is not something I added by the way, I just found out when adding arguments for testing.

Most changes to the code are only executed if you're on OSX. The standard code has been left intact and should cause no problems. I did notice however that autoquotation of file paths was not working when shebang was activated. I made a small change to the existing code in case shebang was used. There might be another way of adding the quotes here, for me this works ok. Now filenames with spaces are always loaded correctly.

I have also updated the README.md file for Mac, and corrected some general typos.

As this is my first contribution to Github I hope I did everything ok, please let me know if you have any comments or need more information. Great package by the way! 👍

pohmelie commented 7 years ago

Actually a talk about switching by platform at load time, like https://github.com/blueimp/atom-open-terminal-here/blob/master/index.coffee#L46 So, you don't need to change logic.

enishii commented 7 years ago

I have added the code to create defaults for Mac (empty) and windows (based on example in README). For Mac some changes are still necessary 'under the hood' though, so some of the updated logic remains.

pohmelie commented 7 years ago

This is not what I expect… but I understood the problem now. You need to close quotation string and you can't do it, since you need different constructions for «open file» and «open directory». I think (since we have solution for mac) we need to break backward compatibility for more flexible settings set. Instead of terminal with arguments and terminal execution argument we need launch file in terminal, launch directory in terminal and new interpolation argument {launcher}, which will represent resolved by extension command. Also, I think we should remove autoquotation, since user can quote all paths himself.

By the way, I think we need cd {working_directory} part of command for files. Since if not, what will be the current working directory?

enishii commented 7 years ago

Beside the different commands on Mac for opening and showing directory you also need to add an additional end to the command. That's why I have added cmd_mac_end in the code, only for use on mac. Since there really is not other way than opening and running a command in the terminal, I suggest to leave these commands in the code, instead of having the users to fill them in fields. One missing ' and it won't work anymore. Other platforms are much easier I think. Therefore if the current setup works ok for *nix and windows I would suggest to leave the backward compatibility intact and not create new fields, unless it has advantages on other platforms too.

For a mac the cwd is not relevant it seems. (nothing changes when I enable/disable this) What is the effect of this on other systems? I see it is used in the child_process.exec command somehow?

Also how is the F6 option supposed to work? Where do the extra arguments come from if this is used?

pohmelie commented 7 years ago

I would suggest to leave the backward compatibility intact and not create new fields, unless it has advantages on other platforms too

Actually, this changes are enhancement since brings more flexibility. Count of fields will stay as is, we remove two fields and add two fields. Update for users on win and mac is transparent, they should not change anything in settings. So, if not you, I will do this thing and get all the fame :laughing:

For a mac the cwd is not relevant it seems.

No, no, no. I'm talking about this scenario:

hurr.py:

with open("hurr.txt", "w") as fout:
    fout.write("durr")

In which place hurr.txt appears? My prediction is foo/hurr.txt, but user expect foo/bar/baz/hurr.txt and this can be achieved via cd foo/bar/baz && python hurr.py.

enishii commented 7 years ago

I can give it a shot (only for the fame ;), but I'm not sure if I understand the code well enough to be successful. This is also the first time I'm programming anything in coffeescript by the way. :) If I don't succeed I'll let you know.

I solved the cwd for mac by adding "cd {working_directory};" in the terminal execution argument field. When I run /foo/bar/helloworld.py the command send to the terminal is: cd "/foo/bar"; python "/foo/bar/helloworld.py" That way the working directory is changed before running the script. && instead of ; has the same effect by the way.

pohmelie commented 7 years ago

@jnelissen

I solved the cwd for mac by adding "cd {working_directory};"

Yes, that is exactly what I mean :+1:

enishii commented 7 years ago

Sorry for the delay, but please have a look at the latest commit. It works great here! I have updated the README as well to reflect the latest status. Users can now use the {launchers} field for interpolation to the selected program. I have also added {args} to the parameters list, which you can add anywhere in the command if you wish.

pohmelie commented 7 years ago

I will test this a little on win and make release soon. Great work! :clap:

pohmelie commented 7 years ago

@jnelissen Ok, I add some stuff, check it out. I think mac os default configuration should escape {working_directory} and {launcher} too.

enishii commented 7 years ago

Working fine here. Do you mean {working_directory} should be in quotes? It is already?

pohmelie commented 7 years ago

Oh, my bad. {working_directory} is, but {launcher} is not. That what I mean. Launcher can be something like /home/dart weider/.pyenv/versions/3.6.1/bin/python. Check out my last commit, is done properly?

enishii commented 7 years ago

In that case it is needed indeed. Your update is correct.

pohmelie commented 7 years ago

Published. Thanks again! It looks like this fix #10 and #16 :clap: