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

first prototype of a stream plugin #304

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

anarcat
Copy link
Contributor

@anarcat anarcat commented Feb 21, 2018

The approach taken is to reuse the existing index.html template and instead flatten album hierarchies in a rather unorthodox way. Media stay linkable by prefixing the album path to their internal paths.

Many issues with the code still:

  • the way albums are flattened is definitely out of whack, with nasty copy() all over the place. not really sure it actually keeps the original albums sane. (update: unit tests make sure the original albums are kept intact)
  • performance might break down on large albums. (is this really a problem?)
  • class methods that should be in the Album class are just lying around stupidly in the plugin instead
  • code refactoring, once done, should trickle back into the feeds plugin as well
  • i am unsure if the modification of filename and thumbnail_path is sufficient for our needs. is url, for example, used anywhere?
  • no unit tests: only tested this by hand and by looking at the ouput.
  • documentation missing (maybe? are docstrings enough?)

Other todos:

  • only one stream is created. having per-album streams might seem redundant, but for large hierarchies, it could prove useful.
  • stream page is not linked from anywhere. i haven't figured out how this is done in the feeds plugin yet and can hack around this in the album descriptions for now.
  • album exclusion could prove useful to avoid the privacy leaks that feeds suffer from

Closes: #303 (eventually)

@anarcat anarcat mentioned this pull request Feb 21, 2018
@codecov
Copy link

codecov bot commented Feb 21, 2018

Codecov Report

Merging #304 into master will increase coverage by 0.17%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
+ Coverage   87.28%   87.46%   +0.17%     
==========================================
  Files          19       20       +1     
  Lines        1408     1452      +44     
==========================================
+ Hits         1229     1270      +41     
- Misses        179      182       +3
Impacted Files Coverage Δ
sigal/gallery.py 88.88% <89.47%> (+0.02%) ⬆️
sigal/plugins/stream.py 92% <92%> (ø)
sigal/plugins/watermark.py 93.02% <0%> (+2.32%) ⬆️

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 a7841c5...1886167. Read the comment docs.

@anarcat
Copy link
Contributor Author

anarcat commented Feb 27, 2018

note that i have found that reordering the test suite may lead to breakage. by using -x -ff to repeatedly test the streams plugin, i managed to trigger a failure in test_cli.py (KeyError("adjust_options") or something). i have lost the backtrace now unfortunately, but this is something to keep in mind: maybe i introduced a regression somehow?

@anarcat
Copy link
Contributor Author

anarcat commented Feb 27, 2018

i have now mostly complete unit test coverage for the code that's actually there, and i updated the todo list in the summary. what's basically missing now is a way to link to (or include?) the stream page somewhere. the feeds plugin is embedded directly in the theme templates:

          {% if 'sigal.plugins.feeds' in settings.plugins %}
          {% if settings.rss_feed %}<br><a href={{ settings.rss_feed.feed_url }}>RSS Feed</a>{% endif %}
          {% if settings.atom_feed %}<br><a href={{ settings.atom_feed.feed_url }}>Atom Feed</a>{% endif %}{% endif %}</p>

should i just do the same, in the same place?

we also need to decide if we want a single stream page or a per-directory one, which would complicate the above template...

i think album exclusion can be implemented in a later stage, unless you think it's absolutely necessary.

@capocasa
Copy link

Just discovered this and loved it!

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.

streams page
2 participants