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

apastel · 2024-12-22T03:26:25Z

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 · 2024-12-22T03:30:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.03%. Comparing base (cdd4e38) to head (19ce896).
Report is 2 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.

sigma67

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 · 2024-12-22T19:29:27Z

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 · 2024-12-23T06:18:22Z

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 reviewed Dec 22, 2024

View reviewed changes

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

19ce896

If a relative file path is provided, `as_uri()` throws a ValueError. Resolve the path to be absolute before converting to URI.

apastel force-pushed the patch-1 branch from c85509b to 19ce896 Compare December 22, 2024 07:27

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

apastel commented Dec 22, 2024

codecov bot commented Dec 22, 2024 •

edited

Loading

sigma67 left a comment

apastel commented Dec 22, 2024

sandrotosi commented Dec 23, 2024

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

Are you sure you want to change the base?

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

Conversation

apastel commented Dec 22, 2024

codecov bot commented Dec 22, 2024 • edited Loading

Codecov Report

sigma67 left a comment

Choose a reason for hiding this comment

apastel commented Dec 22, 2024

sandrotosi commented Dec 23, 2024

codecov bot commented Dec 22, 2024 •

edited

Loading