#16 change_issue_state: add "close_status" option
Merged by yangl1996. Opened by mbasti.
mbasti/libpagure add-close-status  into  master

Download 16.patch

close_status option allows libpagure users to specifiy reason why issue has been closed

rebased

I suggest adding unit tests, but LGTM otherwise.

Should it be an error if close_status is not None and new_status != "closed"?

Likewise, would it be an error if close_status is None and new_status == "closed"?

close_status may be None it is optional field. I don't think I should enforce it more, I would leave what is allowed to pagure REST API as this is just wrapper to its API

rebased

LGTM, we can go ahead and merge when @bowlofeggs approves.

LGTM

I checked the Pagure source code and found that "close_status is not None and new_status != "closed"" won't result in an error, at least in the form processing.

So IMO we don't need to validate here.

LGTM. Thank you very much for the pull request.

Merged.

Pull-Request has been merged by yangl1996

Metadata