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

Allow sorting on metadata keys. #202

Merged
merged 6 commits into from May 2, 2016
Merged

Allow sorting on metadata keys. #202

merged 6 commits into from May 2, 2016

Conversation

@tohojo
Copy link
Contributor

@tohojo tohojo commented Apr 27, 2016

This makes it possible to specify metadata keys for the albums_sort_attr and medias_sort_attr configuration options. This is done by prefixing the key with 'meta.', so e.g. albums_sort_attr = 'meta.order' will sort albums on the 'order' metadata key (collected from the .md files in each directory).

Currently, there's no fallback so the metadata key has to exist for all albums/images and wasn't sure how to add tests (there doesn't seem to be any metadata handling in the test cases at all currently?). If you can provide some feedback on how best to do this I can update the pull request :)

This makes it possible to specify metadata keys for the albums_sort_attr
and medias_sort_attr configuration options. This is done by prefixing
the key with 'meta.', so e.g. albums_sort_attr = 'meta.order' will sort
albums on the 'order' metadata key (collected from the .md files in each
directory).

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
@tohojo tohojo mentioned this pull request Apr 27, 2016
@saimn
Copy link
Owner

@saimn saimn commented Apr 28, 2016

Thanks for working on this @tohojo !
There are some tests here: https://github.com/saimn/sigal/blob/master/tests/test_gallery.py#L167 (medias_sort_attr is tested, but not albums_sort_attr it seems).
It would be nice also if it doesn't fail if some md file doesn't have the meta. Maybe with something like
.meta.get(meta_key, [''])[0], which will make a long and ugly line, but ... And it will put the items without the meta key at the beginning which is not ideal.

@tohojo
Copy link
Contributor Author

@tohojo tohojo commented May 1, 2016

Okay, added some tests. Adding an index.md file to the 'test2' directory in the test tree broke another test, so changed the behaviour of the read_markdown() function.

This is obviously a genuine change of behaviour, but it made sense to me: Why would simply having an index.md file with no title defined not still fall back to the directory name? However, if you prefer I do something else to fix the tests, let me know and I can change it :)

@saimn
Copy link
Owner

@saimn saimn commented May 1, 2016

Adding an index.md file to the 'test2' directory in the test tree broke another test, so changed the behaviour of the read_markdown() function.

Yes indeed, it was a bug so your fix is fine.

@@ -191,6 +191,18 @@ def test_albums_sort(settings):
a.sort_subdirs('title')
assert [im.title for im in a.albums] == list(reversed(titles))

orders = [d.meta['order'] for d in a.albums]

This comment has been minimized.

@saimn

saimn May 1, 2016
Owner

This list could be hard-coded as it is the reference to do the test. It would avoid to rely on the metadata parsing code which is used later in the sort.

This comment has been minimized.

@tohojo

tohojo May 1, 2016
Author Contributor

That is true; can do.

settings['albums_sort_reverse'] = False
a = Album('dir1', settings, album['subdirs'], album['medias'], gal)
a.sort_subdirs('meta.order')
assert [d.meta['order'] for d in a.albums] == orders

This comment has been minimized.

@saimn

saimn May 1, 2016
Owner

d.meta['order'][0] is maybe safer, I don't know if and how py.test tests recursively list of lists.

This comment has been minimized.

@tohojo

tohojo May 1, 2016
Author Contributor

I think the comparison is fine; Python considers two lists to be equal if all their elements are. But if I'm hardcoding the list anyway, it's probably better to change it :)

@saimn
Copy link
Owner

@saimn saimn commented May 1, 2016

If you can handle the case when one of the md files does not have the meta used for the sort, then we are good. Also please add yourself to the AUTHORS files ;). Thanks !

@tohojo
Copy link
Contributor Author

@tohojo tohojo commented May 1, 2016

Ah yes, totally forgot about that. Will do!

tohojo added 5 commits May 1, 2016
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
This fixes the test breakage from adding an index.md with only an Order
key defined.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
This test also tests the missing meta key functionality.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
@tohojo tohojo force-pushed the tohojo:meta-sort branch from be46e4c to aa0a09a May 1, 2016
@tohojo
Copy link
Contributor Author

@tohojo tohojo commented May 1, 2016

Updated... Should be good to go now :)

@saimn saimn merged commit eabcd4a into saimn:master May 2, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@saimn
Copy link
Owner

@saimn saimn commented May 2, 2016

Great, thanks @tohojo !

@saimn saimn added this to the 1.2.0 milestone May 2, 2016
@tohojo tohojo deleted the tohojo:meta-sort branch May 2, 2016
kontza pushed a commit to kontza/sigal that referenced this pull request Aug 28, 2020
Allow sorting on metadata keys.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants