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

fix(setup): Use resolve() before invoking as_uri() #700

Merged
merged 1 commit into from
Dec 27, 2024

Conversation

apastel
Copy link
Contributor

@apastel apastel commented Dec 22, 2024

If no --file argument is provided to setup, as_uri() throws a ValueError when it tries to get the relative path to print to the console. Using resolve() will work in both case (--file provided or not provided).

Fixes #699

Copy link

codecov bot commented Dec 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.03%. Comparing base (cdd4e38) to head (19ce896).
Report is 3 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #700      +/-   ##
==========================================
- Coverage   95.08%   95.03%   -0.05%     
==========================================
  Files          40       40              
  Lines        2358     2358              
==========================================
- Hits         2242     2241       -1     
- Misses        116      117       +1     
Flag Coverage Δ
unittests 95.03% <100.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@sigma67 sigma67 left a comment

Choose a reason for hiding this comment

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

Not instead, in addition. Don't change the behavior of the print statement - with this change the link won't be clickable in terminal

Please also add a test

If a relative file path is provided, `as_uri()` throws a ValueError.
Resolve the path to be absolute before converting to URI.
@apastel apastel changed the title fix(setup): Use resolve() instead of as_uri() for relative pathing fix(setup): Use resolve() before invoking as_uri() Dec 22, 2024
@apastel
Copy link
Contributor Author

apastel commented Dec 22, 2024

I'm inclined to say the test is that you run ytmusicapi browser|oauth with a relative file path and that it doesn't throw an error anymore. I spent some time trying to test the argparse CLI and it's not very intuitive like it is with click, the CLI library that I'm used to, and there aren't yet any examples in the project of tests that actually execute the main() function without mocking it. I don't have the bandwidth to dive into it today beyond what I tried already.

@sandrotosi
Copy link

in my local installation i ended up just removing as_uri(), then i found out about this PR lol anyhow i think the cli to setup authentication being broken is more important than a file link being clickable and should be fixed fast, just my 2c

@sigma67 sigma67 merged commit 4081404 into sigma67:main Dec 27, 2024
5 checks passed
@apastel apastel deleted the patch-1 branch December 28, 2024 01:45
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.

1.9.0: ytmusicapi oauth results in "ValueError: relative path can't be expressed as a file URI"
3 participants