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

RFC: inline: make the album/item available directly #5439

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kergoth
Copy link
Contributor

@kergoth kergoth commented Sep 26, 2024

Description

There have been multiple requests, in the past, for the ability to use plugin fields in inline fields. This has not previously been available. From what I can tell, it was intentionally left unavailable due to performance concerns.

The way the item fields are made available to the inline python code means that all fields are looked up, whether they're actually used by the code or not. Doing that for all computed fields would be a performance concern.

I don't believe there's a good way to postpone the field computation, as python eval and compile requires that globals be a dictionary, not a mapping. Instead, we can make available the album or item model object to the code directly, and let the code access the fields it needs via that object, resulting in postponing the computation of the fields until they're actually accessed.

This is a simple approach that makes the computed and plugin fields available to inline python, which allows for more code reuse, as well as more options for shifting logic out of templates and into python code.

In items, the object is available as 'item', and in albums, it's available as 'album'.

Examples:

item_fields:
  test_file_size: item.filesize

album_fields:
  test_album_path: album.path
  # If the missing plugin is enabled
  test_album_missing: album.missing

To Do

  • Either select new names to refer to the objects, or revisit the approach.
  • 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.)

Copy link

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.

@kergoth
Copy link
Contributor Author

kergoth commented Sep 26, 2024

It looks like there aren't any tests for the inline plugin at all right now, so I'll need to add a module for that to test this. It might be a couple days before I get to that.

@kergoth kergoth force-pushed the inline-model-object branch 2 times, most recently from 5b53ea6 to df58c24 Compare September 26, 2024 03:00
@kergoth
Copy link
Contributor Author

kergoth commented Sep 26, 2024

Updated to reformat with ruff and add the examples to the commit message to align with the PR description.

@kergoth kergoth force-pushed the inline-model-object branch from df58c24 to a92e317 Compare September 26, 2024 03:04
@kergoth
Copy link
Contributor Author

kergoth commented Sep 26, 2024

Updated to add the changelog entry, and link to the missing plugin in the example in the docs. I thought about adding a direct example to the 'examples of expressions' in the inline docs, but the examples in the commit message are contrived, whereas the examples in the docs are more realistic.

There have been multiple requests, in the past, for the ability to use
plugin fields in inline fields. This has not previously been available.
From what I can tell, it was intentionally left unavailable due to
performance concerns.

The way the item fields are made available to the inline python code
means that all fields are looked up, whether they're actually used by
the code or not. Doing that for all computed fields would be a
performance concern.

I don't believe there's a good way to postpone the field computation, as
python eval and compile requires that globals be a dictionary, not a
mapping. Instead, we can make available the album or item model object
to the code directly, and let the code access the fields it needs via
that object, resulting in postponing the computation of the fields until
they're actually accessed.

This is a simple approach that makes the computed and plugin fields
available to inline python, which allows for more code reuse, as well as
more options for shifting logic out of templates and into python code.

In items, the object is available as 'item', and in albums, it's
available as 'album'.

Examples:

    item_fields:
      test_file_size: item.filesize

    album_fields:
      test_album_path: album.path
      # If the missing plugin is enabled
      test_album_missing: album.missing

Signed-off-by: Christopher Larson <[email protected]>
@kergoth kergoth force-pushed the inline-model-object branch from a92e317 to 8692b30 Compare September 26, 2024 03:17
@kergoth
Copy link
Contributor Author

kergoth commented Sep 26, 2024

On second thought, this will break compatibility on the album_fields, as album will change from the album name (str) to the album object (Album), so it'd probably be best to change the name we use to refer to the object. Thoughts? obj seems a bit too programming-like for a plugin that's meant to be more accessible.

@kergoth kergoth changed the title inline: make the album/item available directly RFC: inline: make the album/item available directly Sep 26, 2024
@kergoth
Copy link
Contributor Author

kergoth commented Sep 28, 2024

@sampsyo Any thoughts on this? I think this is probably the best method to make available computed fields without adding delays due to evaluating them all immediately, but I'm not sure on the best naming.

@snejus
Copy link
Member

snejus commented Dec 2, 2024

This looks like a very clean solution. I'm aware that there aren't any tests - would you like to add some? This would be ideal, and they can document what does your change actually achieve!

@kergoth
Copy link
Contributor Author

kergoth commented Dec 3, 2024

This looks like a very clean solution. I'm aware that there aren't any tests - would you like to add some? This would be ideal, and they can document what does your change actually achieve!

Thanks @snejus, I was intending to write tests, but I was needing input regarding my previous comment. Currently, I made the item object available for item fields as item and album fields as album, but album is already used for the name of the album, not the object, so either we need to make it available under a different name, or change the approach. Thoughts on just making it available as item for both contexts, or any other suggested names to use for the object?

@snejus
Copy link
Member

snejus commented Dec 4, 2024

What about something like db_obj? I don't see why the name shouldn't be shared - item_fields and album_fields keys make it clear what's the entity we're dealing with.

Regarding field access in the templates, it is sort of possible to the model instances, at least in album_fields templates. Albums have the items field which has the list of items in that album - so you could do something like items[0]._cached_album and you've got your album.

Not sure if the same trick is possible from item_fields templates, though.

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.

2 participants