Skip to content

permissions: set album art permissions - #4036

Merged
wisp3rwind merged 3 commits into
beetbox:masterfrom
Duncaen:permissions-art
Sep 12, 2021
Merged

permissions: set album art permissions#4036
wisp3rwind merged 3 commits into
beetbox:masterfrom
Duncaen:permissions-art

Conversation

@Duncaen

@Duncaen Duncaen commented Aug 30, 2021

Copy link
Copy Markdown
Contributor

Description

This change makes the permissions plugin set the album art file permissions to the configurable file permissions whenever the set_art event is triggered.

To Do

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

@sampsyo

sampsyo commented Aug 31, 2021

Copy link
Copy Markdown
Member

Looks good overall; thanks!! I have a couple of comments:

  • It would be great to reduce the duplication with the fix method—in particular, the logging and such. Can we make a new helper method to cut down on the copied code?
  • Could you please add a changelog entry? (I don't think this needs other documentation.)
  • I'm also not sure whether the tests would be easily extended to cover album art, but it's worth a quick glance to see.

@Duncaen
Duncaen force-pushed the permissions-art branch 2 times, most recently from e1f60cb to 3377288 Compare August 31, 2021 13:12
@wisp3rwind

Copy link
Copy Markdown
Member

This is looking good to me, what do you think @sampsyo ?

@sampsyo sampsyo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the ping; yes, this all looks great! Nice work. Looks like it needs an update and then should be good to go.

@Duncaen

Duncaen commented Sep 12, 2021

Copy link
Copy Markdown
Contributor Author

Thanks, rebased and fixed the change log conflict.

@wisp3rwind
wisp3rwind merged commit 7548820 into beetbox:master Sep 12, 2021
arogl added a commit to arogl/beets that referenced this pull request Sep 26, 2021
@snejus snejus added the permissions permissions plugin label Apr 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

permissions permissions plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants