Improve readability of pkgdb.lib.vcs_acls by using proper variable names #166
Conversation
| if pkg[0] not in output: | ||
| output[pkg[0].encode('utf-8')] = {} | ||
| if pkgname not in output: | ||
| output[pkgname.encode('utf-8')] = {} |
There was a problem hiding this comment.
Why 'utf-8' here? Would it make sense to keep it unicode while in this loop and then finally convert to byte strings at the very end?
There was a problem hiding this comment.
I think that's a left over from when I tried to build the JSON by hand using a dict structure.
|
Okay, another suggestion. This function contains two giant pieces on either side of if oformat == 'json':
return _do_vcs_acls_json(arguments)
else:
return _do_vcs_acls_the_other_format(arguments)Then the docstring of each of those utility methods could describe what they're |
|
sure can do |
| name: pkg1, | ||
| branch: branch1, | ||
| user: [user1, user2], | ||
| group: [group1, group2] |
There was a problem hiding this comment.
This looks like it should be groups not group and people not user (from reading the code below).
There was a problem hiding this comment.
No no, the key for groups is singular, see https://github.com/fedora-infra/pkgdb2/pull/166/files#diff-87909695400e927cca78825103e7f028R2139 and above :)
There was a problem hiding this comment.
No, no. See for _json it is plural https://github.com/fedora-infra/pkgdb2/pull/166/files#diff-87909695400e927cca78825103e7f028R2075 but for _text it is singular: https://github.com/fedora-infra/pkgdb2/pull/166/files#diff-87909695400e927cca78825103e7f028R2139
There was a problem hiding this comment.
Oh sorry, yes I was looking at the wrong method
|
👍! |
|
Thanks @ralphbean |
Improve readability of pkgdb.lib.vcs_acls by using proper variable names
Fixes #165