Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#917 Rultor should not ask architect to confirm merge on a closed PR #1048

Closed
wants to merge 3 commits into from
Closed

#917 Rultor should not ask architect to confirm merge on a closed PR #1048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
164e201
#917 Rultor should not ask architect to confirm merge on a closed PR
dskalenko Mar 9, 2016
e6cd2fb
#917 Rultor should not ask architect to confirm merge on a closed PR
dskalenko Mar 9, 2016
03afa49
#917 Rultor should not ask architect to confirm merge on a closed PR …
dskalenko Mar 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 43 additions & 33 deletions src/main/java/com/rultor/agents/Agents.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,40 +288,50 @@ public Agent agent(final Talk talk, final Profile profile)
* @return Array of questions.
*/
private static Question commands(final Profile profile) {
return new QnByArchitect(
profile,
"/p/entry[@key='architect']/item/text()",
new QnFirstOf(
new QnIfContains(
"unlock",
new QnUnlock()
),
new QnIfContains(
"lock",
new QnLock()
),
new QnIfContains(
"merge",
new QnAskedBy(
profile,
Agents.commanders("merge"),
new QnIfPull(new QnIfUnlocked(new QnMerge()))
)
),
new QnIfContains(
"deploy",
new QnAskedBy(
profile,
Agents.commanders("deploy"),
new QnDeploy()
return new QnFirstOf(
new QnIfContains(
"merge",
new QnIfPull(
new QnIfUnlocked(
new QnByArchitect(
profile,
"/p/entry[@key='architect']/item/text()",
new QnAskedBy(
profile,
Agents.commanders("merge"),
new QnMerge()
)
)
)
),
new QnIfContains(
"release",
new QnAskedBy(
profile,
Agents.commanders("release"),
new QnRelease()
)
),
new QnByArchitect(
profile,
"/p/entry[@key='architect']/item/text()",
new QnFirstOf(
new QnIfContains(
"unlock",
new QnUnlock()
),
new QnIfContains(
"lock",
new QnLock()
),
new QnIfContains(
"deploy",
new QnAskedBy(
profile,
Agents.commanders("deploy"),
new QnDeploy()
)
),
new QnIfContains(
"release",
new QnAskedBy(
profile,
Agents.commanders("release"),
new QnRelease()
)
)
)
)
Expand Down
35 changes: 31 additions & 4 deletions src/main/java/com/rultor/agents/github/qtn/QnIfPull.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,16 +77,43 @@ public Req understand(final Comment.Smart comment,
final URI home) throws IOException {
final Issue.Smart issue = new Issue.Smart(comment.issue());
final Req req;
if (issue.isPull()) {
req = this.origin.understand(comment, home);
} else {
if (QnIfPull.isNotPull(issue)) {
new Answer(comment).post(
false,
QnIfPull.PHRASES.getString("QnIfPull.not-pull-request")
);
req = Req.EMPTY;
req = Req.DONE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dskalenko Why did you change this from Req.EMPTY to Req.DONE ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@original-brownbear
I changed it since it was required by new logic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dskalenko Ok but why does the new logic require this kind of change? Why adjust anything to the logic of handling this command being issued in a non-pull request while fixing the logic pertaining to actual pull requests?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@original-brownbear
I don't know why QnIfPull returned EMPTY Req at the beginning, because according to how it used QnIfPull should return Req.DONE in case of some incorrectness with issue (like for QnByArchitect). Why we have to proceed
flow forward if command not possible (see QnFirstOf )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dskalenko ok let me rephrase the question:

Please explain clear cut why in order to not have this code act on a closed PR we have to adjust the return signature for the merge command response outside of an actual PR ?
It seems to me that these things are fully unrelated.

} else if (QnIfPull.isClosed(issue)) {
new Answer(comment).post(
false,
QnIfPull.PHRASES.getString("QnIfPull.already-closed")
);
req = Req.DONE;
} else {
req = this.origin.understand(comment, home);
}
return req;
}

/**
* Is issue not pull request.
* @param issue The issue
* @return TRUE if issue not pull request
* @throws IOException If there is any I/O problem
*/
private static boolean isNotPull(final Issue.Smart issue)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dskalenko what's the point of adding this method if we already have the method isPull on the issue ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@original-brownbear
I was trying to avoid PMD warning suppress

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dskalenko what warning ? Can you please give the specific warning you're not getting by using this but getting when directly using the issue methods ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@original-brownbear
sorry didn't specify
Avoid if (x != y) ..; else ..; (ConfusingTernary)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dskalenko just flip the if and else then.

if(x!=y) {foo} else {bar} => if(x==y){bar} else {foo}

throws IOException {
return !issue.isPull();
}

/**
* Is issue closed.
* @param issue The issue
* @return TRUE if issue closed
* @throws IOException If there is any I/O problem
*/
private static boolean isClosed(final Issue.Smart issue)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dskalenko same as in the above, this one is fully redundant, it adds a bunch of lines and confusion only. Just use the issue itself when checking if it's open. You should obviously only add private static utility methods to add logic, not as a glorified negation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@original-brownbear
I was trying to avoid PMD warning suppress , the same as for isPull

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dskalenko same answer as in the above.

throws IOException {
return !issue.isOpen();
}
}
40 changes: 15 additions & 25 deletions src/main/java/com/rultor/agents/github/qtn/QnMerge.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,31 +68,21 @@ public final class QnMerge implements Question {
public Req understand(final Comment.Smart comment,
final URI home) throws IOException {
final Issue.Smart issue = new Issue.Smart(comment.issue());
final Req req;
if (issue.isPull() && issue.isOpen()) {
new Answer(comment).post(
true,
String.format(
QnMerge.PHRASES.getString("QnMerge.start"),
home.toASCIIString()
)
);
Logger.info(
this, "merge request found in %s#%d, comment #%d",
issue.repo().coordinates(), issue.number(), comment.number()
);
req = QnMerge.pack(
comment,
issue.repo().pulls().get(issue.number())
);
} else {
new Answer(comment).post(
false,
QnMerge.PHRASES.getString("QnMerge.already-closed")
);
req = Req.EMPTY;
}
return req;
new Answer(comment).post(
true,
String.format(
QnMerge.PHRASES.getString("QnMerge.start"),
home.toASCIIString()
)
);
Logger.info(
this, "merge request found in %s#%d, comment #%d",
issue.repo().coordinates(), issue.number(), comment.number()
);
return QnMerge.pack(
comment,
issue.repo().pulls().get(issue.number())
);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/main/resources/phrases_en_US.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
QnIfPull.not-pull-request=It is an issue, not a pull request

QnMerge.start=OK, I'll try to merge now. You can check the progress of the merge [here](%s)
QnMerge.already-closed=The pull request is closed already, so I can't merge it
QnIfPull.already-closed=The pull request is closed already, so I can't merge it
QnMerge.head-is-gone=Head repository is gone, can't merge from it
QnMerge.base-is-gone=Base repository is gone, can't merge into it

Expand Down
Loading