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

Config rename #1583

Merged
merged 5 commits into from
Nov 26, 2022
Merged

Config rename #1583

merged 5 commits into from
Nov 26, 2022

Conversation

ildyria
Copy link
Member

@ildyria ildyria commented Nov 7, 2022

No description provided.

@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #1583 (23044ec) into apply-rights-naming-convention (bd1329d) will not change coverage.
The diff coverage is 60.00%.

Additional details and impacted files

@ildyria ildyria requested a review from nagmat84 November 8, 2022 13:08
@kamil4
Copy link
Contributor

kamil4 commented Nov 8, 2022

I've been keeping my distance from the whole naming discussion because currently I have neither the time nor, frankly, the patience for it, but when I see a separate PR of manageable length I can't help but think about it. So, at the danger of engaging in bikeshedding, here are my resolutely uninformed thoughts:

  • Yes, the old names (full_photo, downloadable) are awful.
  • The new names (grants_*) are better.
  • In this particular context though, are they descriptive enough? These config variables are used only in the relatively rare cases where the settings of the parent album do not apply. So wouldn't it be better to call them something like grants_full_photo_access_fallback and grants_download_fallback to indicate their limited role?
  • There are at least two more config variables used in a similar context: share_button_visible and public_photos_hidden. Wouldn't this be the right opportunity to unify their naming as well?

@ildyria
Copy link
Member Author

ildyria commented Nov 8, 2022

grants_full_photo_access_fallback and grants_download_fallback to indicate their limited role?

Then grants_full_photo_access_default and grants_download_default would be better then IMHO. :)

There are at least two more config variables used in a similar context: share_button_visible and public_photos_hidden. Wouldn't this be the right opportunity to unify their naming as well?

Sure can do. :)
In the meantime, may I interested you in #1584 :D

@ildyria ildyria added this to the 4.6.3 milestone Nov 21, 2022
@ildyria ildyria requested a review from qwerty287 November 26, 2022 15:10
@qwerty287
Copy link
Contributor

What happened to

There are at least two more config variables used in a similar context: share_button_visible and public_photos_hidden. Wouldn't this be the right opportunity to unify their naming as well?

?

@ildyria ildyria merged commit 80cacc9 into apply-rights-naming-convention Nov 26, 2022
@ildyria ildyria deleted the config-rename branch November 26, 2022 19:20
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