search

Justintime50 / github-archive

A powerful tool to concurrently clone, pull, or fork user and org repos and gists to create a GitHub archive.
MIT License
186 stars 48 forks source link

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

Closed danieldjewell closed 2 years ago

danieldjewell commented 2 years ago

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....

Justintime50 commented 2 years ago

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.

Justintime50 commented 2 years ago

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 commented 2 years ago

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 :grin: ) ... 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 commented 2 years ago

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: https://github.com/Justintime50/github-archive/blob/b050473703c67dcb131c849d7ef156418ff50adb/github_archive/cli.py#L12

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 commented 2 years ago

Changed. See https://github.com/Justintime50/github-archive/pull/35/commits/8b3646049cfe8b0f9744cd44965232a0cda77cad

Justintime50 commented 2 years ago

Keep an eye on https://github.com/Justintime50/github-archive/pull/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.