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

issue #30 /tags should give a list of tags #39

Merged
merged 1 commit into from May 2, 2016

Conversation

Projects
None yet
3 participants
@voila
Copy link
Contributor

voila commented Apr 30, 2016

A lot of edits are a result of running ocp-indent, and just change the indentation... I hope that's OK.

@hannesm

This comment has been minimized.

Copy link
Collaborator

hannesm commented Apr 30, 2016

cool. would be easier to review if you'd separate the ocp-indent in a separate commit (same PR is fine)

@voila voila force-pushed the voila:tags branch from 7b136a4 to e43ca9f Apr 30, 2016

@voila

This comment has been minimized.

Copy link
Contributor

voila commented Apr 30, 2016

Not sure if this fixed the indentation problem.

I have annotated the relevant changes, in the commit, to make it easier to review.

I am unsure of what I should do re:code formating, in the future... I use Emacs M-x untabify because some files in the master branch have tabs in them, but that also seem to change the indentation. So I used ocp-indent as @Engil suggested, but the resulting indentation is different too, adding a lot of noise to my commits.


let compare a b = Ptime.compare (date b) (date a)

let updated = function
| Markdown m ->
m.Canopy_article.updated

let tags content_map =

This comment has been minimized.

@voila

voila Apr 30, 2016

Contributor

a new function that returns all existings tags (as string list) from the cache map (populated in Store.fill_cache)

a ~a:[a_href article.uri; a_class ["list-group-item"]] (content ++ abstract)

let to_tyxml_tags tags =

This comment has been minimized.

@voila

voila Apr 30, 2016

Contributor

A function to format the list of tags as links to related articles, as HTML. This is used to generates the page for the /tags URI

@@ -66,67 +66,72 @@ module Make (S: Cohttp_lwt.Server) (C: V1_LWT.CONSOLE) (Disk: V1_LWT.KV_RO)
| uri::[] when uri = config.Canopy_config.push_hook_path ->
store.update () >>= fun l ->
respond_update l
| "tags"::[] -> (

This comment has been minimized.

@voila

voila Apr 30, 2016

Contributor

The dispatch rules for the /tags URI

| "tags"::[] -> (
let tags = Canopy_content.tags !cache in
let content = Canopy_article.to_tyxml_tags tags in
respond_html ~headers ~title:config.Canopy_config.blog_name ~content ~updated

This comment has been minimized.

@hannesm

hannesm May 1, 2016

Collaborator

hmm, the updated here is the Canopy startup time... but tags might be added later (by addng a new article, or new tags to an existing article). should we either drop or have the post hook not only refill the !cache, but also remember the timestamp (to be used only here (and maybe for the atom feed))?

This comment has been minimized.

@voila

voila May 1, 2016

Contributor

good spotting... I guess it is silly to recalculate this from the cache, every time...
Should the cache be extended to also store the last tag timestamp, the last article timestamp, the list of tags, etc... ?

This comment has been minimized.

@hannesm

hannesm May 1, 2016

Collaborator

this is not very clear to me... atm the cache contains the articles, but no caching of listings or tags is done... maybe they should be cached (as you might have noticed here I prefer to have as little done as possible on each individual request)?

For this change request here, I'd just use the store.last_commit () >>= fun updated, the same used for the atom feed. Caching can come in a separate PR, but @Engil should tell his opinion :)

This comment has been minimized.

@voila

voila May 1, 2016

Contributor

I am happy to change the code to use store.last_commit () >>= fun updated for this PR, but what if the last commit was not a new tag but a new article ? Shouldn't the Etag (for the /tags page) reflects the set of existing tags only ?

This comment has been minimized.

@hannesm

hannesm May 1, 2016

Collaborator

yep, that's an conservative approximation and will result in more data being transferred in the worst case than needed... but better than an underapproximation which leads to outdated data on the clients.. ;) a new article might as well introduce new tags, as might a change to an existing article.. the logic how to get the last update to any tag (add/remove) is non-trivial

This comment has been minimized.

@voila

voila May 2, 2016

Contributor

Sounds good to me :)

This comment has been minimized.

@Engil

Engil May 2, 2016

Owner

I think at some point we should improve the cache mechanism (which is a Map other Canopy_content.t right now), abstracting thing in such a way we can cache easily other things than articles.
I'm not too sure on how to do that properly, for now this solution works fine enough, we'll see later for something more clever

@voila voila force-pushed the voila:tags branch from e43ca9f to 0bbd3b6 May 2, 2016

@voila

This comment has been minimized.

Copy link
Owner

voila commented on canopy_dispatch.ml in 0bbd3b6 May 2, 2016

Now with updated coming from store.last_commit ()

@hannesm

This comment has been minimized.

Copy link
Collaborator

hannesm commented May 2, 2016

lgtm

@Engil

This comment has been minimized.

Copy link
Owner

Engil commented May 2, 2016

Everything looks fine to me, tested it just now, looks great! (was a bit busy this week end again, sorry for being late to the party…)
Thank you @hannesm for reviewing everything, and thank you @voila again for delivering such nice PRs…
Merging. :)

@Engil Engil merged commit 2ad57e6 into Engil:master May 2, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment