Incorrect assumption that all media_fields are fixed fields #5581
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses the incorrect assumption that all media fields are fixed by updating the logic that determines which fields to update when no fields are provided. Key changes include:
- Removing the dependency on library.Item._media_fields.
- Using library.Item._fields.keys() to obtain the list of all fields.
- Ensuring that the 'path' field is included when the move option is enabled.
| # no fields were provided, update all media fields | ||
| item_fields = fields or library.Item._media_fields | ||
| # no fields were provided, update all fields | ||
| item_fields = fields or library.Item._fields.keys() |
There was a problem hiding this comment.
Using library.Item._fields.keys() returns a view which may not support the add method later when 'move' is true. Consider converting it to a modifiable type (e.g., a set) before attempting to add the 'path' field.
| item_fields = fields or library.Item._fields.keys() | |
| item_fields = fields or set(library.Item._fields.keys()) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
Copilot is right — this needs to be a set, see the next line where another field is added into it.
|
How is this going to prevent the It would be good to include a note in the changelog as well |
Description
Fixes #5580
(...)
To Do
Documentation. (If you've added a new command-line flag, for example, find the appropriate page underdocs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)