Skip to content

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

Merged
sigma67 merged 1 commit intosigma67:mainfrom
apastel:patch-1
Dec 27, 2024
Merged

fix(setup): Use resolve() before invoking as_uri()#700
sigma67 merged 1 commit intosigma67:mainfrom
apastel:patch-1

Conversation

@apastel
Copy link
Copy Markdown
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

@codecov
Copy link
Copy Markdown

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 17 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
@apastel apastel deleted the patch-1 branch December 28, 2024 01:45
kaneryu pushed a commit to kaneryu/ytmusicapi that referenced this pull request Jul 2, 2025
If a relative file path is provided, `as_uri()` throws a ValueError.
Resolve the path to be absolute before converting to URI.
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