Start Peer Review transition

[andrewnicols]

I've tried to write this such that the state changes can be abstracted, but I'm not sure how best this will work for fields updated at the same time.

[FMCorz]

Thanks Andrew,

That's a nice improvement, just a couple of points before I integrate it:

• This should be rebased on top of master, there are already included commits
• This is not necessary https://github.com/andrewnicols/mdk/commit/fcc96ca02599add76fac9db53459aacbab2871e6
• In general the help message of the arguments does not start with a capital, I just followed the style of the default help messages defined by argparse.
• The dest property of the arguments are a mix of camelCase and not, I think they are always lowercase, let's stick to that.
• When I integrated your other pull request I changed the code so that we do not display what changes were made to the issue, we will throw an exception if it did not work. Also at the end of the process we display the issue info so that should confirm that it worked. Can you adapt the code to just apply the transitions, etc... without keeping track of the changes?
• I did not find an instance where you are using issue['namedmappding'], is that required? Is should be enough to have the named fields and raw fields. We can also use the config file to track specific fields.
• Minor but I generally order the methods alphabetically.
• In getTransitions() you could use the function parameter params to set the expand value. See getIssue().
• makeTransition() exception does not need to include the status, but it could say something like "Could not transition the issue to '...'".
• makeTransition() can now only return True
• I often forget that, but when defining the arguments of makeTransition() it's preferrable to leave fields/update as None otherwise the objects will stick to the function. Say you set update['test'], next call it will still be set. I don't think this is causing any issue in your patch, it's just something that I noticed and very often forget.
• getComment() does not belong to the Jira API as it invokes an editor, I would suggest asking for a comment when --comment is set, or when we are changing the transition (but from the command file, not the API).
• In reviewStart() you could make the existing peer reviewer check happen before requesting the transitions, it saves a call if the PR is already defined. I was considering placing that check in the command itself, but it's probably find there, we could add a parameter safe=false to the function later. (Same applies to reviewFail()). I just noticed that we could only request the peer reviewer field rather than all, though we would need to know the custom name of it so it's fine.
• reviewStart() could support a comment, but let's leave that for later.
• reviewFail() should use a comment parameter rather than getComment().
• reviewFail() does not seem to use namedMappings.
• In general can we rename the methods reviewStart/Fail to peerReviewStart/Fail? That's more specific.
• Can you isolate the commit fixing tools.launchEditor?
• addComment() also should not use getNewComment() or return something else than True. The exception does not need to include more information about the issue and status.
• Wrong comment in developmentStart/Stop(), it mentions reviewer.
• In what circumstances is it useful to use --list-transitions? I'd assume that you should know what transitions are available by just looking at the status of the issue.

Thanks Andrew, this looks like many comments but they are all pretty minor really. Cheers for working that, that is going to be fun to use.

Nice work! Fred

[FMCorz]

Hi Andrew,

are you able to extract the commits for Peer review and Dev and rebase them on master?

Cheers, Fred