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

feature: it works, is ugly #30

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ dev = [
"black",
"pytest-cov",
"flake8",
"requests_mock"
]

[tool.black]
Expand Down
57 changes: 47 additions & 10 deletions shbin.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import re
import secrets
import sys
import requests
from mimetypes import guess_extension

import pyclip
Expand Down Expand Up @@ -60,6 +61,21 @@ def get_repo_and_user():
return gh.get_repo(os.environ["SHBIN_REPO"]), gh.get_user().login


def get_repo(full_name_repo):
gh = Github(os.environ["SHBIN_GITHUB_TOKEN"])
return gh.get_repo(full_name_repo)


def get_path(url):
pattern = r"https://github.com/.*?/(?:tree|blob)/(.*)"
result = re.findall(pattern, url)
Comment on lines +70 to +71
Copy link
Collaborator

Choose a reason for hiding this comment

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

do not use findall for a single potential match. also it would be a little better to compile the pattern at module level

PATTERN = re.compile(r"https://github.com/.*?/(?:tree|blob)/(.*)")

def get_path(url):
    try:
         return re.match(pattern, url).group(1)
    except ...     

if result:
extracted_string = result[0]
return extracted_string
else:
raise DocoptExit(f"Ensure your path is from github repository. (error {e})")
Comment on lines +71 to +76
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here e in the message is not defined.

Also, it's considered more pythonic the EAFP approach

So, it should something like

Suggested change
result = re.findall(pattern, url)
if result:
extracted_string = result[0]
return extracted_string
else:
raise DocoptExit(f"Ensure your path is from github repository. (error {e})")
try:
return re.findall(pattern, url)[0]
except TypeError:
raise DocoptExit(f"Ensure the URL is from github repository")



def expand_paths(path_or_patterns):
"""
receive a list of relative paths or glob patterns and return an iterator of Path instances
Expand Down Expand Up @@ -99,21 +115,42 @@ def download(url_or_path, repo, user):
$ shbin dl https://github.com/Shiphero/pastebin/blob/main/bibo/AWS_API_fullfilment_methods/
$ shbin dl bibo/AWS_API_fullfilment_methods/
"""
path = re.sub(rf"^https://github\.com/{repo.full_name}/(blob|tree)/{repo.default_branch}/", "", url_or_path)
full_name_repo = repo.full_name
pattern = r"https://github.com/(.*?)/(tree|blob)/"
result = re.findall(pattern, url_or_path)
if result:
full_name_repo = result[0][0]

is_from_my_repo = "https://" not in url_or_path or full_name_repo == repo.full_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

a few things here.

  • the first condition should be not url_or_path.startswith("https://")

  • is it enough to assume that if it doesn't start with https:// then is from repo?

  • wording: "my_repo" . I'm the user but the repo could someone else and I just have permissions, right? like mgaitan downloading from Shiphero/pastebin. I would use is_from_the_repo or something like that

if is_from_my_repo:
path = re.sub(rf"^https://github\.com/{repo.full_name}/(blob|tree)/{repo.default_branch}/", "", url_or_path)
else:
repo = get_repo(full_name_repo)
path = get_path(url_or_path)
Comment on lines +125 to +129
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we are trying to parse with a regex, couldn't we generalize it an accept urls or user/repo/path forms for any repo and then check if it's the our not?

suppose shbin zauberzeug/nicegui/examples


path = path.rstrip("/")
try:
content = repo.get_contents(path)
if isinstance(content, list):
# FIXME currently this will flatten the tree:
# suposse dir/foo.py and dir/subdir/bar.py
# Then `$ shbin dl dir` will get foo.py and bar.py in the same dir.
for content_file in content:
download(content_file.path, repo, user)
return
if is_from_my_repo:
content = repo.get_contents(path)
if isinstance(content, list):
# FIXME currently this will flatten the tree:
# suposse dir/foo.py and dir/subdir/bar.py
# Then `$ shbin dl dir` will get foo.py and bar.py in the same dir.
for content_file in content:
download(content_file.path, repo, user)
return
else:
content = content.decoded_content
else:
content = content.decoded_content
url = f"https://raw.githubusercontent.com/{repo.full_name}/{path}"
response = requests.get(url)
if response.status_code > 200:
raise Exception("There was a problem with your download, please check url")
content = response.content
Comment on lines +145 to +149
Copy link
Collaborator

@mgaitan mgaitan Jun 21, 2023

Choose a reason for hiding this comment

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

what if we unify own or foreign contents with requests? the only difference it that you need to pass the token in the header.

In [16]: requests.get("https://raw.githubusercontent.com/Shiphero/pastebin/main/mgaitan/15e1VFq87UA.txt",
    ...:     headers={"Authorization": f"token {os.environ['SHBIN_GITHUB_TOKEN']}", "Accept": "application/vnd.github.v3.raw'"}
    ...: )
Out[16]: <Response [200]>

In [17]: Out[16].content[:144]
Out[17]: b"    \n>       mock_sns.push.assert_called_once_with(topic_arn, message)\nE       AssertionError: expected call not found.\nE       Expected: push('"

the good news is that passing a token to a public content doesn't affect, so this could be the only code

In [20]: requests.get("https://raw.githubusercontent.com/facebook/react/main/README.md",
    ...:     headers={"Authorization": f"token {os.environ['SHBIN_GITHUB_TOKEN']}", "Accept": "application/vnd.github.v3.raw'"}
    ...: )
Out[20]: <Response [200]>

except GithubException:
print("[red]x[/red] content not found")
except Exception as e:
print(f"[red]x[/red] {e}")
Comment on lines +152 to +153
Copy link
Collaborator

Choose a reason for hiding this comment

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

which other exception could be?

else:
target = pathlib.Path(path).name
pathlib.Path(target).write_bytes(content)
Expand Down
42 changes: 41 additions & 1 deletion tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,17 @@ def repo():
repo = create_autospec(Repository, name="gh_repo")
repo.create_file.return_value = {"content": Mock(html_url="https://the-url")}
repo.update_file.return_value = {"content": Mock(html_url="https://the-url-updated")}
repo.full_name = "my_awesome/repository"
return repo


@pytest.fixture
def outside_repo():
outside_repo = create_autospec(Repository, name="gh_repo")
outside_repo.full_name = "another_awesome/repository"
return outside_repo


@pytest.fixture(autouse=True)
def pyclip(monkeypatch):
class Stub:
Expand Down Expand Up @@ -70,6 +78,12 @@ def patched_repo_and_user(repo):
yield mocked


@pytest.fixture
def patched_another_repo_and_user(outside_repo):
with patch("shbin.get_repo", return_value=outside_repo) as mocked:
yield mocked


@pytest.mark.parametrize("argv", (["-h"], ["--help"]))
def test_help(capsys, argv):
with pytest.raises(SystemExit):
Expand Down Expand Up @@ -320,7 +334,7 @@ def test_force_new(pyclip, tmp_path, patched_repo_and_user, repo, capsys):
assert capsys.readouterr().out == "🔗📋 https://the-url-2\n"


def test_download_a_file(tmp_path, patched_repo_and_user, repo):
def test_download_a_file_from_owned_repo(tmp_path, patched_repo_and_user, repo):
git_data = {
"decoded_content": b"awesome content",
}
Expand All @@ -330,3 +344,29 @@ def test_download_a_file(tmp_path, patched_repo_and_user, repo):
os.chdir(working_dir)
main(["dl", "hello.md"])
assert (working_dir / "hello.md").read_bytes() == b"awesome content"


def test_download_a_file_from_public_repo(
tmp_path, patched_another_repo_and_user, outside_repo, requests_mock, patched_repo_and_user
):
requests_mock.get(
"https://raw.githubusercontent.com/another_awesome/repository/main/hello.md", content=b"awesome content"
)
working_dir = tmp_path / "working_dir"
working_dir.mkdir()
os.chdir(working_dir)
main(["dl", "https://github.com/another_awesome/repository/blob/main/hello.md"])
Comment on lines +357 to +358
Copy link
Collaborator

Choose a reason for hiding this comment

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

it would nice to have an option to define where to download the contents, like

shbin dl URL  [-o  path] 

assert (working_dir / "hello.md").read_bytes() == b"awesome content"


def test_download_a_file_from_public_repo_rasie_error(
tmp_path, patched_another_repo_and_user, outside_repo, requests_mock, patched_repo_and_user
):
requests_mock.get("https://raw.githubusercontent.com/another_awesome/repository/main/hello.md", status_code=400)
working_dir = tmp_path / "working_dir"
working_dir.mkdir()
Comment on lines +366 to +367
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't need a subfolder. tmp_path is already a temporary Path instance.

os.chdir(working_dir)
Copy link
Collaborator

@mgaitan mgaitan Jun 21, 2023

Choose a reason for hiding this comment

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

not a good practice to change the dir in this way. When needed, use monkeypatch.chdir so it's undo automatically on teardown (even if the test fails or errored). https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest.MonkeyPatch.chdir

main(["dl", "https://github.com/another_awesome/repository/blob/main/hello.md"])
with pytest.raises(Exception) as exc_info:
raise Exception("There was a problem with your download, please check url")
assert str(exc_info.value) == "There was a problem with your download, please check url"
Comment on lines +370 to +372
Copy link
Collaborator

Choose a reason for hiding this comment

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

this has no sense Alvar. It's a self-fulfilling prophecy: "I will prove that the following code raise an exception " and the following code is... literally raise Exception().