Skip to content

Conversation

@JOJ0
Copy link
Member

@JOJ0 JOJ0 commented Oct 21, 2025

Description

Updates the docs chapter "Handling Paths" describing how to modernise old code and intentionally includes historical details. Examples should further guide contributors while refactoring.

To Do

  • Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
  • Tests. (Very much encouraged but not strictly required.)

Copilot AI review requested due to automatic review settings October 21, 2025 07:01
@JOJ0 JOJ0 requested a review from a team as a code owner October 21, 2025 07:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR rewrites the "Handling Paths" chapter in the contributing documentation to reflect the transition from legacy path utilities (syspath(), normpath(), bytestring_path(), displayable_path()) to modern pathlib.Path. The documentation now includes historical context explaining why the old utilities existed, guidance on when to use each approach, and concrete examples showing old vs. new patterns.

  • Adds historical context for legacy path utilities and their original purposes
  • Introduces pathlib.Path as the preferred approach for new code
  • Provides side-by-side examples comparing old and new path handling patterns

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.00%. Comparing base (52b102c) to head (528d5e6).
⚠️ Report is 4 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6116      +/-   ##
==========================================
- Coverage   67.22%   67.00%   -0.23%     
==========================================
  Files         118      118              
  Lines       18180    18180              
  Branches     3079     3079              
==========================================
- Hits        12221    12181      -40     
- Misses       5299     5340      +41     
+ Partials      660      659       -1     

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@semohr
Copy link
Contributor

semohr commented Oct 21, 2025

How do we feel about moving this into the developer documentation? I don't think it should be included in the contribution guide.

@JOJ0
Copy link
Member Author

JOJ0 commented Oct 21, 2025

How do we feel about moving this into the developer documentation? I don't think it should be included in the contribution guide.

Good idea! I keep forgetting n starting to look for it there anyway!! 😂

@JOJ0 JOJ0 force-pushed the docs_handling_paths branch 2 times, most recently from c1ffa87 to d56962a Compare October 23, 2025 20:19
@JOJ0
Copy link
Member Author

JOJ0 commented Oct 23, 2025

@semohr moved it to the dev docs but now realize that actually this is the plugin writing section. It kind of fits there but is not only related to plugins but a general thing. Have a better idea where to put it or is fine there? Thanks for any feedback on the wording too!

@semohr
Copy link
Contributor

semohr commented Oct 24, 2025

Awesome, although why not just create a file in docs/dev directly? I don't think the path handling is plugin specific or do you feel like this needs to be under the plugins umbrella for a specific reason?

Otherwise I'm happy with the new sections. I don't think the wording is that important for the dev docs in general. It is a major improvement either way and we can always iterate it if something feels off.

@snejus
Copy link
Member

snejus commented Oct 25, 2025

This makes me think that we should extend the pathlib.Path in our codebase and perform the expanduser and resolve resolution on init automatically. I think this would make it much easier to use it going forward. What do you think?

@JOJ0 JOJ0 force-pushed the docs_handling_paths branch 2 times, most recently from 83f7a7c to 8215eaf Compare October 25, 2025 21:23
@JOJ0
Copy link
Member Author

JOJ0 commented Oct 25, 2025

This makes me think that we should extend the pathlib.Path in our codebase and perform the expanduser and resolve resolution on init automatically. I think this would make it much easier to use it going forward. What do you think?

You mean for the .filepath property? Good idea!

Related question, do we recommend to store as bytes in such cases as well, I just recommended it that way over there: https://github.com/beetbox/beets/pull/4748/files#diff-5ddf2bcdeaa73deda8e015085ca50330be268bd9352cfda7ed68bcd0879b8c51R56

So basically I'm asking: In case we store paths not for Items or Albums but anything else, In flexible fields for example. We still want to store that as bytes, right? (In that particular example item.path returns bytes already, so simply storing it as-is in that flexible field doesnt require wrapping through bytestring_path)

@JOJ0 JOJ0 force-pushed the docs_handling_paths branch from 8215eaf to 3c2c050 Compare October 28, 2025 06:25
@JOJ0
Copy link
Member Author

JOJ0 commented Oct 28, 2025

Awesome, although why not just create a file in docs/dev directly? I don't think the path handling is plugin specific or do you feel like this needs to be under the plugins umbrella for a specific reason?

Nope, as mentioned already, I don't think it fits under plugins :-)

Otherwise I'm happy with the new sections. I don't think the wording is that important for the dev docs in general. It is a major improvement either way and we can always iterate it if something feels off.

Not a native-speaker here but IMHO at least it should be halfway good english - even for dev docs :-) I get goosebumps when reading all wrongly worded documentation - no matter if for developers or endusers. Haha! ;-)

You are right we can always change it if something is not 100% correct.

I finally found a good place for the chapter in the main docs level. Right between "Library Database API" and "Music importer". the former talks about Item and Album objects, following the paths chapter which talks about the .path and .file_path properties seems to fit perfectly.

Also I reworded and clarified this paragraph:

Screenshot 2025-10-28 at 07 24 23

Added it to the changelog and merging it now. Thanks for the feedback! @semohr ah but please give me an approval so I can press the button. Thanks!

@JOJ0 JOJ0 force-pushed the docs_handling_paths branch from 79cd611 to 2cbb2d7 Compare October 28, 2025 06:42
@semohr semohr enabled auto-merge October 28, 2025 09:58
@JOJ0 JOJ0 force-pushed the docs_handling_paths branch from 2cbb2d7 to 528d5e6 Compare October 28, 2025 11:56
@semohr semohr merged commit adc0d9e into master Oct 28, 2025
21 checks passed
@semohr semohr deleted the docs_handling_paths branch October 28, 2025 12:02
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.

3 participants