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 new plugin to allow one to disable ZIP generation on a per album basis #368

Merged
merged 10 commits into from Aug 26, 2019

Conversation

riton
Copy link
Contributor

@riton riton commented May 14, 2019

Heavily inspired by the .nomedia file, this patch introduces a new plugin called nozip_gallery.

If zip_gallery is enabled, this plugin allows to control on a per album basis if a ZIP file should be generated or not.

It currently uses a .nozip_gallery file (empty or not) to achieve such behavior.

The motivation behind this plugin is:

  • disable ZIP file generation for my very old photo albums
  • enable ZIP file generation for my recent photo albums

…basis

* Heavily inspired by the '.nomedia' file
@codecov
Copy link

@codecov codecov bot commented May 14, 2019

Codecov Report

Merging #368 into master will decrease coverage by 0.76%.
The diff coverage is 13.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
- Coverage   87.38%   86.62%   -0.77%     
==========================================
  Files          18       19       +1     
  Lines        1443     1458      +15     
==========================================
+ Hits         1261     1263       +2     
- Misses        182      195      +13
Impacted Files Coverage Δ
sigal/plugins/nozip_gallery.py 0% <0%> (ø)
sigal/gallery.py 88.88% <66.66%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 912ca39...43f07a7. Read the comment docs.

@codecov
Copy link

@codecov codecov bot commented May 14, 2019

Codecov Report

Merging #368 into master will increase coverage by 0.19%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
+ Coverage   87.38%   87.58%   +0.19%     
==========================================
  Files          18       19       +1     
  Lines        1443     1466      +23     
==========================================
+ Hits         1261     1284      +23     
  Misses        182      182
Impacted Files Coverage Δ
sigal/gallery.py 89.52% <100%> (+0.48%) ⬆️
sigal/plugins/zip_gallery.py 90.24% <90.24%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 912ca39...a3bf898. Read the comment docs.

@riton
Copy link
Contributor Author

@riton riton commented May 14, 2019

Note:

Another solution would have been to rely on the album index.md file with a GenerateZIP: true / false setting.

I've chosen to use a .nozip_gallery file like the .nomedia file for consistency.

@saimn
Copy link
Owner

@saimn saimn commented May 22, 2019

Hi,
Thanks for the PR, and sorry for the delay. This would indeed be a nice feature to have. Thinking about it, we could maybe move the zip feature to the plugin, that would then take care of adding the zip method to the albums when the plugin is activated and when the special file does not exists. Would you be interested to have a look at this?
Also it will need some unit tests :)

* ZIP generation logic is now self contained in a plugin
* '.nozip_gallery' file logic is now contained in the same plugin that
  managed the zip generation
@riton
Copy link
Contributor Author

@riton riton commented Jun 3, 2019

Hi, my turn to take a long time to respond, sorry about that.

I've made some modifications and moved the whole zip generation logic to a zip_gallery plugin.
Is this what you meant / wanted ?

About the unit tests, I'll see if I can find some time to write them. Before going further, I want to be sure that we're on the same page about the zip logic modifications that I've made in fc14de3

@riton
Copy link
Contributor Author

@riton riton commented Jun 3, 2019

BTW @saimn , just noticed that you're from Lyon. If you want to meet and discuss further in real life next to a beer or so, I'd be glad 😉

".nozip_gallery file", album.name)
zip_method = noop_zip_method

album.__class__.zip = cached_property(zip_method)
Copy link
Owner

@saimn saimn Jun 4, 2019

Choose a reason for hiding this comment

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

Since the property is set on the class, it will be used for all the Album objects (the last will win). So instead I think that you can check for the "nozip" file directly in generate_album_zip_method ?

Copy link
Contributor Author

@riton riton Jun 5, 2019

Choose a reason for hiding this comment

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

You're absolutely right, this was too naive. I've switched to a single function that does all the checks and contains all the logic.

archive.close()
self.logger.debug('Created ZIP archive %s', archive_path)
return zip_gallery
return None
Copy link
Owner

@saimn saimn Jun 4, 2019

Choose a reason for hiding this comment

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

I wonder if having the placeholder method is necessary? Jinja evaluates missing attributes to an empty string - I think, so it should be fine if the .zip method is missing.

Copy link
Contributor Author

@riton riton Jun 5, 2019

Choose a reason for hiding this comment

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

I don't think this is necessary. I just thought this can make the code easier to read.
For the reader, this placeholder may give a hint on where the actual implementation of the zip property is made. (I'm not a huge fan of magic properties that just appears you don't know where from even if python allows it 😄 )

Anyway, If you'd like me to remove the placeholder, no problem for me. I can simply replace it with a comment to help future readers.

Copy link
Owner

@saimn saimn Jun 13, 2019

Choose a reason for hiding this comment

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

I see your point about the magic aspect and agree in general, but on the other side it is weird to have some unrelated zip stuff here, since this is now an independent feature. And the implementation will be easy to find in the zip plugin.

sigal/plugins/zip_gallery.py Outdated Show resolved Hide resolved
@saimn
Copy link
Owner

@saimn saimn commented Jun 4, 2019

Thanks @riton , I think this is a very nice improvement!
For the test, there is already a test_zip.py file, so the easiest would to add a "nozip" file in some directory in tests/sample/pictures, and check that the zip archive for this directory is not created (in test_zipped_correctly).
And yes, I would be happy to have a beer in Lyon :)

@riton
Copy link
Contributor Author

@riton riton commented Jun 5, 2019

@saimn I've made some quick modifications following your suggestions. Still no unit tests for now.

docs/plugins.rst Outdated
@@ -119,6 +119,11 @@ Nomedia plugin

.. automodule:: sigal.plugins.nomedia

No ZIP Gallery plugin
Copy link
Owner

@saimn saimn Jun 13, 2019

Choose a reason for hiding this comment

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

Suggested change
No ZIP Gallery plugin
ZIP Gallery plugin

The plugin is more general now ;)

archive.close()
self.logger.debug('Created ZIP archive %s', archive_path)
return zip_gallery
return None
Copy link
Owner

@saimn saimn Jun 13, 2019

Choose a reason for hiding this comment

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

I see your point about the magic aspect and agree in general, but on the other side it is weird to have some unrelated zip stuff here, since this is now an independent feature. And the implementation will be easy to find in the zip plugin.

Album.zip = cached_property(generate_album_zip)

def register(settings):
signals.album_initialized.connect(nozip_galery_file)
Copy link
Owner

@saimn saimn Jun 13, 2019

Choose a reason for hiding this comment

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

galery -> gallery (and the same above)

album.logger.warn('Failed to add %s to the ZIP: %s', p, e)

archive.close()
album.logger.debug('Created ZIP archive %s', archive_path)
Copy link
Owner

@saimn saimn Jun 13, 2019

Choose a reason for hiding this comment

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

You can use the logger defined at the top of the module instead of album.logger.

@saimn
Copy link
Owner

@saimn saimn commented Jun 13, 2019

@riton - Could you have a look at the test to see why it is failing ?

@saimn saimn added this to the 2.1 milestone Aug 25, 2019
@saimn
Copy link
Owner

@saimn saimn commented Aug 26, 2019

Hi @riton ,
I have finalized the PR with the tests and a few details, so this is now good to go. Thanks for your work !

@saimn saimn merged commit dcefc35 into saimn:master Aug 26, 2019
3 checks passed
kontza pushed a commit to kontza/sigal that referenced this issue Aug 28, 2020
Add new plugin to allow one to disable ZIP generation on a per album basis
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.

None yet

2 participants