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

Add Option to use Anon HTTPS URIs instead of SSH #35

Merged
merged 2 commits into from
Sep 16, 2021

Conversation

danieldjewell
Copy link
Contributor

Without this option, the only URL used to clone a repo was the SSH URL.
This adds a simple flag to switch clone operations to using the HTTPS
URL. The default behavior of using SSH is preserved so as to not break
scripts, etc.

Tested and seems to work just fine.

I tried to follow the conventions already established -- I suppose one improvement would be to move the new arg higher in the parser list....

Without this option, the only URL used to clone a repo was the SSH URL.
This adds a simple flag to switch clone operations to using the HTTPS
URL. The default behavior of using SSH is preserved so as to not break
scripts, etc.
@Justintime50
Copy link
Owner

Thank you for the PR!

Let me think on this, we could probably do something along these lines, I'd want to ensure we don't break stuff with mixed content such as some SSH and some HTML.

Can you for now add trailing commas where possible? The code looks sound, will look it over again shortly to see if there are any edge cases we can protect against.

github_archive/archive.py Outdated Show resolved Hide resolved
github_archive/cli.py Outdated Show resolved Hide resolved
github_archive/cli.py Outdated Show resolved Hide resolved
@Justintime50
Copy link
Owner

For future travelers, here is some context as to why supporting both may be advantageous: https://stackoverflow.com/questions/11041729/why-does-github-recommend-https-over-ssh

@danieldjewell
Copy link
Contributor Author

For future travelers, here is some context as to why supporting both may be advantageous: https://stackoverflow.com/questions/11041729/why-does-github-recommend-https-over-ssh

That's a cool article -- and good guidance.

I personally always use HTTPS if cloning anything public (it also at least seems faster... but I haven't measured it - human perception of time is still in alpha stage 😁 ) ... I wasn't even aware that authenticated HTTPS access was possible.... (For my repos I clone using SSH + keys -- once configured, it's easier for managing pushes.)

Will take a look at the comments and see what I can do - thanks for taking the time to look it over!

@danieldjewell
Copy link
Contributor Author

danieldjewell commented Sep 15, 2021

Yep, I'd be happy to fix those. (I've never been huge on trailing commas personally, but I can see their utility... sorry for not picking up on that originally.)

I never even realized that the topic is mentioned in PEP-8: https://www.python.org/dev/peps/pep-0008/#when-to-use-trailing-commas

In reading about this I discovered that flake8 actually doesn't report the lack of trailing commas by default... but there's a plugin for that! (I ran flake8 before I submitted the PR since that's what was configured as the linter... and was a little surprised when you commented about it and flake8 didn't warn me...)

Here's the plugin... https://github.com/PyCQA/flake8-commas

(I tried it and sure enough, it lints the lack of trailing commas... Adding it to the project should be as simple as adding it to the venv...)

EDIT: The plugin works well, but it looks for trailing commas everywhere... like even here:

Related side note: Have you thought about possibly selecting a code formatter? (Black, yapf, your pick) Would be really nice from a newcomer's perspective to be able to do a make format or something and know that things are consistent. (I try my best to be consistent with the project but I'm human and sometimes miss things :))

@danieldjewell
Copy link
Contributor Author

Changed. See 8b36460

Copy link
Owner

@Justintime50 Justintime50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding those trailing commas! That will help keep future diffs small since someone won't need to add them in when the list grows.

I'll take one more look in the next couple days and plan to get this released shortly.

@Justintime50 Justintime50 merged commit 6850ecb into Justintime50:main Sep 16, 2021
Justintime50 pushed a commit that referenced this pull request Sep 16, 2021
* Add Option to use Anon HTTPS URIs instead of SSH

Without this option, the only URL used to clone a repo was the SSH URL.
This adds a simple flag to switch clone operations to using the HTTPS
URL. The default behavior of using SSH is preserved so as to not break
scripts, etc.

* Add trailing commas
@Justintime50
Copy link
Owner

Keep an eye on #37 as that will be when I release the next version which contains this fix. I'm going to wait a day or two because Coveralls is being weird and I want a clean build at release time.

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

2 participants