Page MenuHomeSoftware Heritage

Add navigation breadcrumbs for contextual revision browsing
ClosedPublic

Authored by jbertran on Jun 24 2016, 3:21 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Commits
R65:6290dd61957a: utils: better management of variables, better branch conditions
R65:93a1f4dccd13: api: use api_lookup with a closure instead of manually emulating api_lookup
R65:cd8bf1260bd0: service, backend: switch to generators from map objects for msgpack
R65:30b58fef2a1e: browse: remove unneeded comments
R65:e52716f74920: utils: more accurate variable names test_utils: simplify and remove unwarranted…
R65:17ce42848f57: utils: move the parent/child context retrieval to their own functions
R65:d897ed4d48d9: browse: take into account the addition of navigation breadcrumbs
R65:5b6543954bdf: templates: display contextual revision navigation where possible
R65:423713e32c6d: utils: add navigation breadcrumbs management to enrich_revision
R65:b51c4c9e22a4: api: take into account the addition of navigation breadcrumbs
rDWAPPS6290dd61957a: utils: better management of variables, better branch conditions
rDWAPPS93a1f4dccd13: api: use api_lookup with a closure instead of manually emulating api_lookup
rDWAPPScd8bf1260bd0: service, backend: switch to generators from map objects for msgpack
rDWAPPS30b58fef2a1e: browse: remove unneeded comments
rDWAPPSb51c4c9e22a4: api: take into account the addition of navigation breadcrumbs
rDWAPPSd897ed4d48d9: browse: take into account the addition of navigation breadcrumbs
rDWAPPSe52716f74920: utils: more accurate variable names test_utils: simplify and remove…
rDWAPPS17ce42848f57: utils: move the parent/child context retrieval to their own functions
rDWAPPS5b6543954bdf: templates: display contextual revision navigation where possible
rDWAPPS423713e32c6d: utils: add navigation breadcrumbs management to enrich_revision
Summary

utils: add navigation breadcrumbs management to enrich_revision
api: take into account the addition of navigation breadcrumbs
browse: take into account the addition of navigation breadcrumbs
templates: display contextual revision navigation where possible
service, backend: switch to generators from map objects for msgpack

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jbertran retitled this revision from to Add navigation breadcrumbs for contextual revision browsing.
jbertran updated this object.
jbertran edited the test plan for this revision. (Show Details)
ardumont added inline comments.
swh/web/ui/backend.py
182 ↗(On Diff #219)

print!

swh/web/ui/tests/test_utils.py
497

The intent of the side-effect url_for_test function (defined in this test) is not to make the test behaves exactly the same as the production code. This would not make sense since this side-effect function is not the production code.

I just want to assert that the enrich mechanism works for a revision without children or parent entry with a relative coherence to the production code (that's why i wanted some sensible similarly like urls than production code here but again it's not necessary).

Furthermore, i see that you changed the test code to factorize (which is often good) but neither the input nor the output changed. So those modifications do not add anything to the test.

If anything, they add complexity to the maintenance (for the next person).
Can you please remove those modifications on that test only.

547

This time, in this context, i agree this is necessary ^^

549

if the else statement is no longer used, please remove it.
Also, if it is removed, the side effect function become simply url_for_context_test.

It'll simplify later maintenance.
^^

598

Same remark as previous comment.

swh/web/ui/service.py
256 ↗(On Diff #219)

Note: This was to be symmetric with the generator below ^^

258 ↗(On Diff #219)

Note: It was a fix because the serialization step (swh.web.ui.backend layer discussion with swh-storage) was not ok with map.

swh/web/ui/tests/test_utils.py
479

Please, prefix it with _ as you did for the other @nottest util test function.
It is a good idea.

swh/web/ui/utils.py
212

I'm sorry but what does that mean?

jbertran added inline comments.
swh/web/ui/utils.py
212

imm for immediate, ie the direct child of the revision, which we can obtain from the breadcrumbs. In hindsight there's no reason not to write "immediate" instead.

jbertran edited edge metadata.

Minor changes from inline comments

swh/web/ui/utils.py
201

At first sight, this has become a hairy ball, can't you please try to split it up some more?
(... Even when reading it, i don't quite grasp it.)

You have defined quite a lot of tests to cover it so it should not be a problem to try and split it (even though it's unit tests).

212

Ok, thanks.
direct is shorter and that's exactly what you used to define it to me ^^
I'd say that's a sign ^^

236

if context: should be sufficient.

290

Well, like i said previously. Please try to split this.

I already thought it was big before this (and there were no nested ifs).
So you can imagine what i think now ^^

Not only the use cases are important, we also need to maintain it.
So the:

  • code's intent must be clear (i don't find it is right now)
  • code's coverage must be fine (it is!)

Hints for you to split up:

  • you already have defined tests so don't touch them for now, they are your specs (if all use cases are covered that is).

At the end of it all, it should still work (if you keep the same endpoint as point of origin).

Question hints:

  • What are all possible contexts?
  • Can't you use one function per context? (the answer can be anything ^^)
swh/web/ui/views/api.py
532 ↗(On Diff #220)

ahhhh, we are missing currying here... (as in ocaml or haskell ^^)

You could actually have defined a closure lambda passing the context.
Something like (not tested):

def _enrich_revision(revision, context=context):  # closes over the context 
    return utils.enrich_revision(revision, context)

return _api_lookup(
    sha1_git,
    lookup_fn=service.lookup_revision,
    error_msg_if_not_found='Revision with sha1_git %s not'
                           ' found.' % sha1_git,
    enrich_fn=_enrich_revision)
swh/web/ui/views/browse.py
346 ↗(On Diff #220)

throught?

348 ↗(On Diff #220)

his/her.

434 ↗(On Diff #220)

is it a todo or fixme?

swh/web/ui/tests/test_utils.py
596

Sorry to not be clearer, just pass self._url_for_context_test to mock_flask.url_for.side_effect instruction below.

ardumont added a reviewer: ardumont.
This revision now requires changes to proceed.Jun 27 2016, 11:52 AM
jbertran added inline comments.
swh/web/ui/tests/test_utils.py
596

That's just me being dense, no worries ^^

jbertran edited edge metadata.
jbertran marked 4 inline comments as done.

utils: move the parent/child context retrieval to their own functions
browse: remove unneeded comments
api: use api_lookup with a closure instead of manually emulating api_lookup

Almost there ^^

swh/web/ui/utils.py
200

I already liked it better ^^, thanks.

218

You could already return here so that you don't have too many indentation in the second branch.

228

Please, extract a variable child_id (or something) before the if (it's used in both branched condition).
As we do not have types, this permits to at least explains the intent of the code snippet.
This also permits to avoid mistyping ^^

Why would it be the last sha1 that is the direct children?
Sorry if it's obvious again, i just don't see it.

253

Please, again, name this context.split thingy.
It makes for a better readability.

Since you use it so much, i think it's a good idea to have an utility function with an explicit name for that.
Its goal would be to split the context (as per input of the api) and return the list of sha1s.
(The order of that list should respect the order of the string context.)
What do you think?

jbertran added inline comments.
swh/web/ui/utils.py
228

The path that contains the navigation breadcrumbs is constructed from the oldest revision to the most recent. So when you're looking up the last element in path, you get the very next revision in the path you took through the revisions.

jbertran marked an inline comment as done.
jbertran edited edge metadata.

utils: better management of variables, better branch conditions

swh/web/ui/utils.py
228

duh! ^^
Indeed.

ardumont edited edge metadata.

Thanks!

This revision is now accepted and ready to land.Jun 27 2016, 4:11 PM
This revision was automatically updated to reflect the committed changes.