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.

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

Made XmlMethodCall public and added a test. #451

Merged
merged 1 commit into from
Apr 21, 2020
Merged

Conversation

vzurauskas
Copy link
Contributor

PR for #443

I made the XmlMethodCall public and added a test. N.B. The "call" elements in skeleton don't have the structure expected in the test, but it will be implemented in #437.

@0crat
Copy link
Collaborator

0crat commented Apr 17, 2020

This pull request #451 is assigned to @fanifieiev/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @paulodamaso/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be a monetary reward for this job

@@ -55,7 +52,7 @@
@Override
public String asString() throws IOException {
return new Joined(
"", this.call.xpath("name/text()").get(0),
"", this.call.xpath("op/name/text()").get(0),

Choose a reason for hiding this comment

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

@vzurauskas Could you please replace get(0) with
org.cactoos.scalar.ItemAt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fanifieiev Can you explain why? If the goal is to get more object oriented by replacing method calls with object composition, this will not help, because ItemAt is a Scalar, and we would still need to call value() on it. This would only introduce an indirection without reducing the number of method calls.

Choose a reason for hiding this comment

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

@vzurauskas Agree

* Test case for {@link XmlMethodCall}.
* @since 0.44
*/
final class XmlMethodCallTest {

Choose a reason for hiding this comment

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

@vzurauskas Why missing public keyword?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fanifieiev JUnit 5 test classes don't have to be public.

Choose a reason for hiding this comment

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

@vzurauskas Good to know, thanks

final class XmlMethodCallTest {

@Test
void hasClassMethodAndArgs() throws IOException {

Choose a reason for hiding this comment

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

@vzurauskas Why missing public keyword?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fanifieiev JUnit 5 test classes don't have to be public.

Choose a reason for hiding this comment

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

@vzurauskas Good to know, thanks

Copy link

@fanifieiev fanifieiev left a comment

Choose a reason for hiding this comment

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

@paulodamaso Please merge

@paulodamaso
Copy link
Collaborator

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Apr 21, 2020

@rultor merge

@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit a0463ca into cqfn:master Apr 21, 2020
@rultor
Copy link
Collaborator

rultor commented Apr 21, 2020

@rultor merge

@paulodamaso Done! FYI, the full log is here (took me 15min)

@0crat
Copy link
Collaborator

0crat commented Apr 22, 2020

@fanifieiev/z this job was assigned to you 5days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

@paulodamaso
Copy link
Collaborator

@0crat status

@0crat
Copy link
Collaborator

0crat commented Apr 23, 2020

@0crat status (here)

@paulodamaso This is what I know about this job in C7JGJ00DP, as in §32:

@paulodamaso
Copy link
Collaborator

@0crat quality acceptable

@fanifieiev
Copy link

@0crat status

@0crat
Copy link
Collaborator

0crat commented Apr 26, 2020

@0crat status (here)

@fanifieiev This is what I know about this job in C7JGJ00DP, as in §32:

@paulodamaso
Copy link
Collaborator

@0crat out

@0crat 0crat added the qa label Apr 26, 2020
@0crat
Copy link
Collaborator

0crat commented Apr 26, 2020

Code review was too long (8 days), architects (@paulodamaso) were penalized, see §55

@0crat 0crat removed the scope label Apr 26, 2020
@0crat
Copy link
Collaborator

0crat commented Apr 26, 2020

@sereshqua/z please review this job completed by @fanifieiev/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed

@sereshqua
Copy link

@fanifieiev please make sure your comments would mostly about design problems, not cosmetic issues during next CR

@fanifieiev
Copy link

@fanifieiev please make sure your comments would mostly about design problems, not cosmetic issues during next CR

@sereshqua OK

@sereshqua
Copy link

@0crat quality acceptable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants