Feature update #20
Feature update #20
Conversation
|
|
||
| def get_build_for_advisory(advisory_id, koji_session, debug=False): | ||
| if debug: | ||
| print('got advisory "{0}"'.format(advisory_id)) |
There was a problem hiding this comment.
flake8 is failing here - a prior version of this patch had .format(errata_id), which was wrong, but it should be passing on this here.
There was a problem hiding this comment.
Ah, no, I just received notification from prior versions in email. This version is fine.
There was a problem hiding this comment.
suggestion (non-blocking): you might consider using f-strings like we do in most of our other code, we have found them to be a nice readabilty improvement over the older .format style. So this would become:
print(f"got advisory {advisory_id}")
| # Automatically resolve input class to dict of: | ||
| # {'n': 'n-e:v-r', 'm': 'm-e:v-r'} | ||
| # Supports: | ||
| # - pungi compose | ||
| # - koji tag | ||
| # - flat file (list of NVRs |
|
This looks like helpful additions. I'm less familiar with the norms and expectations around this repository, do you think it would be possible to add unit tests? And perhaps consider adding doc strings in a few places? |
There was a problem hiding this comment.
Firstly, apologies for the delay in review, I did not see this patch come in, just noticed it because of the recent review from @jpichon . Overall, i think the code changes are good, though I would really like to see this library start to adopt some of the newer style things we use elsewhere in this org (couple examples in comments to give an idea what I mean). I would not hold merge on those, just want to get the thought out there for further improvements.
I 100% agree on the tests and docs @jpichon mentioned. We already have pytest and tox in place, being run by the github CI, so I don't see any reason we should not have tests with each new patch. I find it painful to consider approving a patch with no tests, no matter how sure the author is that everything works great. By docs, we are meaning just doc-strings for the new functions being added. We have found in our other projects these are enormously helpful both for users and reviewers (and even the original author, after they have not looked at the code for 6 months!). So I would kindly ask that you add these 2 things to this patch, we are happy to assist if you run into any problems doing so.
| def fetch_or_read(url): | ||
| if url.startswith('file://'): | ||
| filename = url[7:] | ||
| fp = open(filename, 'r') |
There was a problem hiding this comment.
suggestion: if you do this in a with block it will be more in keeping with current python idioms, and take care of the close for you. It would look like this:
with open(filename, 'r') as fp:
ret = fp.read()
|
|
||
|
|
||
| def json_api_as_nevr(url): | ||
| md = fetch_json_data(url) |
There was a problem hiding this comment.
question: can we use a more descriptive variable name here? Obviously 'md' is going to be the returned json, but maybe we could just spell out what this abbreviation is for, so it is more clear to readers?
|
|
||
| def get_build_for_advisory(advisory_id, koji_session, debug=False): | ||
| if debug: | ||
| print('got advisory "{0}"'.format(advisory_id)) |
There was a problem hiding this comment.
suggestion (non-blocking): you might consider using f-strings like we do in most of our other code, we have found them to be a nice readabilty improvement over the older .format style. So this would become:
print(f"got advisory {advisory_id}")
| except ValueError: | ||
| advisory_list = get_advisory_list(release_name_or_id, debug=debug) | ||
|
|
||
| return _get_builds_for_advisory_list(advisory_list, koji_session, debug=debug) |
There was a problem hiding this comment.
This was a nice separation of chunks of functionality, and might be an ideal place for some of those tests @jpichon suggested.
I've been using some of these changes for some time, so it's time to publish them