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 support windows. #25

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@adiroiban

adiroiban commented Oct 27, 2014

This is based on #23 but without merge errors and with ghp_import module included in the distribution.

Cheers

georgevreilly added a commit to georgevreilly/ghp-import that referenced this pull request Jan 3, 2015

@georgevreilly

This comment has been minimized.

Show comment
Hide comment
@georgevreilly

georgevreilly Jan 3, 2015

I've closed #23, since #25 does a slightly better job and doesn't have merge errors.

georgevreilly commented Jan 3, 2015

I've closed #23, since #25 does a slightly better job and doesn't have merge errors.

@@ -1,5 +1,3 @@
#! /usr/bin/env python

This comment has been minimized.

@d0ugal

d0ugal May 30, 2015

Rather than renaming ghp-import → ghp_import/__init__.py it would probably be better to just do ghp-import → ghp_import.py.

@d0ugal

d0ugal May 30, 2015

Rather than renaming ghp-import → ghp_import/__init__.py it would probably be better to just do ghp-import → ghp_import.py.

@d0ugal

This comment has been minimized.

Show comment
Hide comment
@d0ugal

d0ugal May 30, 2015

@davisp Would you consider this change? Is there anything I can do to help? It is currently causing some errors for MkDocs users. I'm happy to handle this at our end (probably just copying the code we need), but if I can help fix upstream I'd rather do that.

d0ugal commented May 30, 2015

@davisp Would you consider this change? Is there anything I can do to help? It is currently causing some errors for MkDocs users. I'm happy to handle this at our end (probably just copying the code we need), but if I can help fix upstream I'd rather do that.

@aloisdg

This comment has been minimized.

Show comment
Hide comment
@aloisdg

aloisdg Sep 14, 2015

Any progress on this?

I have an issue related.

aloisdg commented Sep 14, 2015

Any progress on this?

I have an issue related.

@d0ugal

This comment has been minimized.

Show comment
Hide comment
@d0ugal

d0ugal Sep 14, 2015

I would recommend just vendoring ghp-import. That is what we did with MkDocs. https://github.com/mkdocs/mkdocs/blob/master/mkdocs/utils/ghp_import.py

d0ugal commented Sep 14, 2015

I would recommend just vendoring ghp-import. That is what we did with MkDocs. https://github.com/mkdocs/mkdocs/blob/master/mkdocs/utils/ghp_import.py

@matthewgilbert

This comment has been minimized.

Show comment
Hide comment
@matthewgilbert

matthewgilbert Feb 9, 2016

I'm wondering if there are plans to merge this in? It would be nice to have support for Windows

matthewgilbert commented Feb 9, 2016

I'm wondering if there are plans to merge this in? It would be nice to have support for Windows

@davisp

This comment has been minimized.

Show comment
Hide comment
@davisp

davisp Feb 9, 2016

Owner

Oops, lost track of this one. I'm not going to merge this as is. Renaming ghp-import to init.py is to gross for me. I'm not a Windows developer so I don't have any insight on how to make that less ugly. If someone finds a better approach I'm not opposed to Windows support.

Owner

davisp commented Feb 9, 2016

Oops, lost track of this one. I'm not going to merge this as is. Renaming ghp-import to init.py is to gross for me. I'm not a Windows developer so I don't have any insight on how to make that less ugly. If someone finds a better approach I'm not opposed to Windows support.

@d0ugal

This comment has been minimized.

Show comment
Hide comment
@d0ugal

d0ugal Feb 9, 2016

The problem is just with the lack of file extension. The comment I made would do the job: #25 (comment)

I can do a new pull request if it will be welcome.

d0ugal commented Feb 9, 2016

The problem is just with the lack of file extension. The comment I made would do the job: #25 (comment)

I can do a new pull request if it will be welcome.

@georgevreilly

This comment has been minimized.

Show comment
Hide comment
@georgevreilly

georgevreilly Feb 9, 2016

The crucial part of this fix is to use console_scripts, which works on both Unix and Windows. On Unix, it creates a tiny wrapper called ghp-import; on Windows, it creates ghp-import.exe.

georgevreilly commented Feb 9, 2016

The crucial part of this fix is to use console_scripts, which works on both Unix and Windows. On Unix, it creates a tiny wrapper called ghp-import; on Windows, it creates ghp-import.exe.

@d0ugal

This comment has been minimized.

Show comment
Hide comment
@d0ugal

d0ugal Feb 10, 2016

Sure, but console scripts doesn't work on a file without a .py extension. Both changes are needed AFAIK.

I wasn't clear in my last comment but I intended to just reply to the issue @davisp had with the file renaming.

d0ugal commented Feb 10, 2016

Sure, but console scripts doesn't work on a file without a .py extension. Both changes are needed AFAIK.

I wasn't clear in my last comment but I intended to just reply to the issue @davisp had with the file renaming.

@davisp

This comment has been minimized.

Show comment
Hide comment
@davisp

davisp Feb 10, 2016

Owner

Renaming ghp-import to ghp-import.py would be fine with me. Do we have to remove the shebang line though? One of the things I've tried to do is make this super easy to include anywhere via just dropping it onto the file system and running it. I don't mind adding the .py extension but a missing shebang line would be confusing to anyone that tried running it as a script directly rather than using the python interpreter. I fell that this is confusing so to be specific:

Breaking this:

$ ./ghp-import

By requiring:

$ python ghp-import

Is something I'd rather avoid.

Owner

davisp commented Feb 10, 2016

Renaming ghp-import to ghp-import.py would be fine with me. Do we have to remove the shebang line though? One of the things I've tried to do is make this super easy to include anywhere via just dropping it onto the file system and running it. I don't mind adding the .py extension but a missing shebang line would be confusing to anyone that tried running it as a script directly rather than using the python interpreter. I fell that this is confusing so to be specific:

Breaking this:

$ ./ghp-import

By requiring:

$ python ghp-import

Is something I'd rather avoid.

@d0ugal

This comment has been minimized.

Show comment
Hide comment
@d0ugal

d0ugal Feb 10, 2016

Do we have to remove the shebang line though?

No, leaving that is fine.

However, the addition of console_scripts in the setup.py will create a ghp-import command for users when it is installed by pip. This will be added to the path too, so the shebang will not really be needed. Leaving the shebang will allow users to call it directly like you demonstrated, so that is fine and is useful in some cases (like when doing development).

d0ugal commented Feb 10, 2016

Do we have to remove the shebang line though?

No, leaving that is fine.

However, the addition of console_scripts in the setup.py will create a ghp-import command for users when it is installed by pip. This will be added to the path too, so the shebang will not really be needed. Leaving the shebang will allow users to call it directly like you demonstrated, so that is fine and is useful in some cases (like when doing development).

@davisp

This comment has been minimized.

Show comment
Hide comment
@davisp

davisp Feb 10, 2016

Owner

I've pushed a commit to master. If someone wants to verify it works on Windows, that would be splendid.

Owner

davisp commented Feb 10, 2016

I've pushed a commit to master. If someone wants to verify it works on Windows, that would be splendid.

@matthewgilbert

This comment has been minimized.

Show comment
Hide comment
@matthewgilbert

matthewgilbert Feb 11, 2016

Ran on windows and this does the trick, thanks.

matthewgilbert commented Feb 11, 2016

Ran on windows and this does the trick, thanks.

@davisp

This comment has been minimized.

Show comment
Hide comment
@davisp

davisp Feb 11, 2016

Owner

Sounds like its fixed. Closing this off.

Owner

davisp commented Feb 11, 2016

Sounds like its fixed. Closing this off.

@davisp davisp closed this Feb 11, 2016

@stuaxo

This comment has been minimized.

Show comment
Hide comment
@stuaxo

stuaxo Nov 25, 2017

http://docs.getpelican.com/en/3.7.1/tips.html

Tips needs to be updated to not mention this pull request

stuaxo commented Nov 25, 2017

http://docs.getpelican.com/en/3.7.1/tips.html

Tips needs to be updated to not mention this pull request

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