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

split code into separate module and tool #79

Closed
wants to merge 4 commits into from

Conversation

satta
Copy link
Contributor

@satta satta commented May 7, 2016

This PR restructures the code of icdiff to provide a separate icdiff module which is installed separately in a Python module directory as well as as script icdiff which is meant to go into the $PATH. This is IMHO another (cleaner?) way to address #72, #73 etc. without having to add the .py extension to the main script.
My changed variant still runs the tests successfully in the local cloned as well as the installed version.
Comments? Maybe there is a more Pythonic way to do this ;)

@satta satta mentioned this pull request May 7, 2016
@jeffkaufman
Copy link
Owner

Currently http://www.jefftk.com/icdiff has "install for mac/linux" with:

curl -s https://raw.githubusercontent.com/jeffkaufman/icdiff/release-1.8.1/icdiff \
  | sudo tee /usr/local/bin/icdiff > /dev/null \
  && sudo chmod ugo+rx /usr/local/bin/icdiff

I'd really like to keep an installation option like this available, where people can run a simple command and end up with an icdiff executable installed.

I'm not really sure what the right way to do this is?

@satta
Copy link
Contributor Author

satta commented May 8, 2016

My approach would be to upload the new split icdiff package to PyPI and thd then change the icdiff web page to suggest using pip:

sudo pip install icdiff

This would install both the module and the executable (into /usr/local/bin by default). It does require pip to be present for installation, but IMHO that's quite common for Python stuff anyway nowadays; the current approach also needs curl as a second dependency.

@vnitinv
Copy link

vnitinv commented May 8, 2016

sudo pip install icdiff

This is already working. With having both option. icdiff working as commandline tool and as module.
We needed to change diff function to work in module way. check this #75

@xrat
Copy link

xrat commented May 8, 2016

I'd also very much prefer that icdiff could be used in both ways. I agree that pip is "quite common nowadays", though, on my production machines I still refuse to run pip as root. Here I prefer single file scripts that I can easily review, install and update by means of http and cat. In fact, I wouldn't be following Jeff's icdiff if it wasn't for the fact that he offered a single file solution.

@satta
Copy link
Contributor Author

satta commented May 8, 2016

Ah, I see (concerning #75)... so if I understand the setup.py correctly, icdiff.py is installed in the module path and an icdiff wrapper script without .py extension is created in the $PATH that just calls the start function? That is indeed cool and a much better solution 👍 Thanks!

@vnitinv
Copy link

vnitinv commented May 8, 2016

You got it right @satta

@satta satta closed this May 8, 2016
@lurch
Copy link

lurch commented Jan 19, 2018

FYI the "proper" way to do this is using setuptools, see http://python-packaging.readthedocs.io/en/latest/command-line-scripts.html and also codespell-project/codespell#136 as this allows it to work from the command-line on all platforms (e.g. by automatically creating icdiff.bat on Windows).

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.

None yet

5 participants