Changeset View
Standalone View
swh/web/ui/utils.py
Show First 20 Lines • Show All 191 Lines • ▼ Show 20 Lines | if 'uuid' in entity: | ||||
entity['uuid_url'] = flask.url_for('api_entity_by_uuid', | entity['uuid_url'] = flask.url_for('api_entity_by_uuid', | ||||
uuid=entity['uuid']) | uuid=entity['uuid']) | ||||
if 'parent' in entity and entity['parent']: | if 'parent' in entity and entity['parent']: | ||||
entity['parent_url'] = flask.url_for('api_entity_by_uuid', | entity['parent_url'] = flask.url_for('api_entity_by_uuid', | ||||
uuid=entity['parent']) | uuid=entity['parent']) | ||||
return entity | return entity | ||||
ardumont: I already liked it better ^^, thanks. | |||||
def enrich_revision(revision, context=None): | def enrich_revision(revision, context=None): | ||||
ardumontUnsubmitted Not Done Inline ActionsAt first sight, this has become a hairy ball, can't you please try to split it up some more? 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). ardumont: At first sight, this has become a hairy ball, can't you please try to split it up some more?
(.. | |||||
"""Enrich revision with links where it makes sense (directory, parents). | """Enrich revision with links where it makes sense (directory, parents). | ||||
Keep track of the navigation breadcrumbs if they are specified. | |||||
Args: | |||||
revision: the revision as a dict | |||||
context: the navigation breadcrumbs as a /-separated string of revision | |||||
sha1_git | |||||
""" | """ | ||||
context_for_parents = None | |||||
context_for_children = None | |||||
url_imm_child = None | |||||
ardumontUnsubmitted Done Inline ActionsI'm sorry but what does that mean? ardumont: I'm sorry but what does that mean? | |||||
jbertranAuthorUnsubmitted Done Inline Actionsimm 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: imm for immediate, ie the direct child of the revision, which we can obtain from the… | |||||
ardumontUnsubmitted Done Inline ActionsOk, thanks. ardumont: Ok, thanks.
`direct` is shorter and that's exactly what you used to define it to me ^^
I'd say… | |||||
if not context: | if not context: | ||||
context = revision['id'] | context_for_parents = revision['id'] | ||||
else: | |||||
context_for_parents = '%s/%s' % (context, revision['id']) | |||||
Done Inline ActionsYou could already return here so that you don't have too many indentation in the second branch. ardumont: You could already return here so that you don't have too many indentation in the second branch. | |||||
prev_for_children = context.split('/')[:-1] | |||||
if len(prev_for_children) > 0: | |||||
context_for_children = '/'.join(prev_for_children) | |||||
# This commit is not the first commit in the path | |||||
if context_for_children: | |||||
url_imm_child = flask.url_for( | |||||
'api_revision', | |||||
sha1_git=context.split('/')[-1], | |||||
context=context_for_children) | |||||
# This commit is the first commit in the path | |||||
Done Inline ActionsPlease, extract a variable child_id (or something) before the if (it's used in both branched condition). Why would it be the last sha1 that is the direct children? ardumont: Please, extract a variable child_id (or something) before the if (it's used in both branched… | |||||
Not Done Inline ActionsThe 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: The path that contains the navigation breadcrumbs is constructed from the oldest revision to… | |||||
Not Done Inline Actionsduh! ^^ ardumont: duh! ^^
Indeed.
| |||||
else: | |||||
url_imm_child = flask.url_for( | |||||
'api_revision', | |||||
sha1_git=context.split('/')[-1]) | |||||
revision['url'] = flask.url_for('api_revision', sha1_git=revision['id']) | revision['url'] = flask.url_for('api_revision', sha1_git=revision['id']) | ||||
revision['history_url'] = flask.url_for('api_revision_log', | revision['history_url'] = flask.url_for('api_revision_log', | ||||
sha1_git=revision['id']) | sha1_git=revision['id']) | ||||
if context is not None: | |||||
ardumontUnsubmitted Not Done Inline Actionsif context: should be sufficient. ardumont: `if context:` should be sufficient. | |||||
revision['history_context_url'] = flask.url_for( | |||||
'api_revision_log', | |||||
sha1_git=revision['id'], | |||||
prev_sha1s=context) | |||||
if 'author' in revision: | if 'author' in revision: | ||||
author = revision['author'] | author = revision['author'] | ||||
revision['author_url'] = flask.url_for('api_person', | revision['author_url'] = flask.url_for('api_person', | ||||
person_id=author['id']) | person_id=author['id']) | ||||
if 'committer' in revision: | if 'committer' in revision: | ||||
committer = revision['committer'] | committer = revision['committer'] | ||||
revision['committer_url'] = flask.url_for('api_person', | revision['committer_url'] = flask.url_for('api_person', | ||||
person_id=committer['id']) | person_id=committer['id']) | ||||
if 'directory' in revision: | if 'directory' in revision: | ||||
Done Inline ActionsPlease, again, name this context.split thingy. Since you use it so much, i think it's a good idea to have an utility function with an explicit name for that. ardumont: Please, again, name this context.split thingy.
It makes for a better readability.
Since you… | |||||
revision['directory_url'] = flask.url_for( | revision['directory_url'] = flask.url_for( | ||||
'api_directory', | 'api_directory', | ||||
sha1_git=revision['directory']) | sha1_git=revision['directory']) | ||||
if 'parents' in revision: | if 'parents' in revision: | ||||
parents = [] | parents = [] | ||||
for parent in revision['parents']: | for parent in revision['parents']: | ||||
parents.append(flask.url_for('api_revision_history', | parents.append(flask.url_for('api_revision', | ||||
sha1_git_root=context, | sha1_git=parent, | ||||
sha1_git=parent)) | context=context_for_parents)) | ||||
revision['parent_urls'] = parents | revision['parent_urls'] = parents | ||||
if 'children' in revision: | if 'children' in revision: | ||||
children = [] | children = [] | ||||
for child in revision['children']: | for child in revision['children']: | ||||
children.append(flask.url_for('api_revision_history', | # child not the breadcrumb-indicated child > new breadcrumb start | ||||
sha1_git_root=context, | if context: | ||||
if child != context.split('/')[-1]: | |||||
children.append(flask.url_for('api_revision', | |||||
sha1_git=child)) | sha1_git=child)) | ||||
else: | |||||
children.append(flask.url_for('api_revision', sha1_git=child)) | |||||
if url_imm_child: | |||||
children.append(url_imm_child) | |||||
revision['children_urls'] = children | revision['children_urls'] = children | ||||
else: | |||||
if url_imm_child: | |||||
revision['children_urls'] = [url_imm_child] | |||||
if 'message_decoding_failed' in revision: | if 'message_decoding_failed' in revision: | ||||
revision['message_url'] = flask.url_for( | revision['message_url'] = flask.url_for( | ||||
'api_revision_raw_message', | 'api_revision_raw_message', | ||||
sha1_git=revision['id']) | sha1_git=revision['id']) | ||||
return revision | return revision | ||||
ardumontUnsubmitted Not Done Inline ActionsWell, like i said previously. Please try to split this. I already thought it was big before this (and there were no nested ifs). Not only the use cases are important, we also need to maintain it.
Hints for you to split up:
At the end of it all, it should still work (if you keep the same endpoint as point of origin). Question hints:
ardumont: Well, like i said previously. Please try to split this.
I already thought it was big before… |
I already liked it better ^^, thanks.