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

Add the possibility to customize Album names by introducing template as new option for Album name. #431

Merged
merged 10 commits into from
Jan 25, 2024

Conversation

WACKYprog
Copy link
Contributor

@WACKYprog WACKYprog commented Jan 22, 2024

What issue type does this pull request address? (keep at least one, remove the others)
/kind feature

resolves #150
resolves #157

Does this pull request add new dependencies?
Add the possibility to customize Album names by introducing template as new option for Album name.

A small parser has been implemented to support:

Tokens:
  %_folderpath% - full path of the folder containing the file.
  %_directory% - name of the folder containing the file.
  %_parent_directory% - Replaced with the name of the parent folder of the file.
  %_day% - day of the month the file was created (in "DD" format).
  %_month% -month the file was created (in "MM" format).
  %_year% - year the file was created (in "YYYY" format).
  %_time% - time the file was created (in "HH:MM:SS" 24-hour format).
  %_time_en% - time the file was created (in "HH:MM:SS AM/PM" 12-hour format).
               
Funcations:
   $lower(string) - converts the string to lowercase.
   $upper(string) - converts the string to uppercase.
   $sentence(string) - converts the string to sentence case.
   $title(string) - converts the string to title case.
   $regex(string, regex, replacement) - replaces the string with the regex replacement.
   $cutLeft(string, length) - cuts the string from the left.
   $cutRight(string, length) - cuts the string from the right.

Example:	"template:%_directory% - %_month%.%_day%.$cutLeft(%_year%,2)"

What to highlight some difference from proposed spec in #150.

  • I spited date to allow easy region/language specific formatting
  • I added time and time_en to allow for PM AM time format as it is common in UK,US, India etc.
  • added more string case conversion and made naming bit more clear

What to highlight some other decision I made

  • parser do supports recursive options so you can do things like: $upper($cutLeft(%_directory%,5))
  • parser support string wrapping in '' so you can do case like $regex(%_directory%, _, ',')
  • datetime is bases on last file modification time. I will likely put another pull request supporting files exif for datetime you think that could be useful. Also with added support for exif more tags can be added.

What else do we need to know?

increase test coverage of album.go to 94%
This is very basic and should be replaced with EXIF data or video metadata.
However, it is a pretty involved change and will be submitted as a separate
pull request after this code is landed.
and add abelity to wrap function argument in quotes ''
This change adds "template" to the list of valid album name formats
and increases the complexity of the config validation.
It looks like it was an intention to keep the config
validation simple, but considering how complex the template itself is,
I think the app should provide more information about what is exactly
wrong with the template so the user can correct it.
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (1b2da8e) 65.25% compared to head (057bab8) 72.03%.

Files Patch % Lines
internal/upload/album.go 99.11% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #431      +/-   ##
==========================================
+ Coverage   65.25%   72.03%   +6.77%     
==========================================
  Files          22       22              
  Lines         947     1173     +226     
==========================================
+ Hits          618      845     +227     
  Misses        305      305              
+ Partials       24       23       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WACKYprog WACKYprog marked this pull request as draft January 22, 2024 20:28
- incressing test coverage for alubm.go
- split parser functions to make it bit more readable
@pacoorozco pacoorozco marked this pull request as ready for review January 23, 2024 06:39
Copy link
Member

@pacoorozco pacoorozco left a comment

Choose a reason for hiding this comment

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

Thank you very much for contributing to this humble project with a feature that had been in the backlog for so long. I think it is something that some people were waiting for.

I have reviewed the code and it looks very good. Good test coverage and very readable code. Thanks for the effort in implementing the feature and trying to keep the code consistent.

I have made some comments, mostly typos. Once you review them ping me and I'll do a merge as well as a new release of the CLI.

Once again, thanks!

@WACKYprog
Copy link
Contributor Author

Thank you very much for contributing to this humble project with a feature that had been in the backlog for so long. I think it is something that some people were waiting for.

I have reviewed the code and it looks very good. Good test coverage and very readable code. Thanks for the effort in implementing the feature and trying to keep the code consistent.

I have made some comments, mostly typos. Once you review them ping me and I'll do a merge as well as a new release of the CLI.

Once again, thanks!

Happy to help.

I use the project and really have fun working on it. Hopefully will have time to work on it more!

Copy link
Member

@pacoorozco pacoorozco left a comment

Choose a reason for hiding this comment

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

Well done and thanks for addressing all the comment 🙇🏼

@pacoorozco pacoorozco merged commit 498245b into gphotosuploader:main Jan 25, 2024
9 of 10 checks passed
@pacoorozco pacoorozco mentioned this pull request Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants