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

(#472) - Remove checkstyle suppression #476

Merged
merged 1 commit into from
May 12, 2020
Merged

Conversation

victornoel
Copy link
Contributor

@victornoel victornoel commented May 9, 2020

This is for #472.

The changes are small, but I verified that:

  • all the tests were present
  • there was no more things to extract from the class
  • that it didn't make sense to transform the build method into a class because we would end up with two classes, one for XmlGraph just delegating to the second, and the second very similar to XmlGraph as it is currently written.

It also gave me the opportunity to understand what the code was doing, and I opened a few issues in cactoos and jpeek.

@victornoel
Copy link
Contributor Author

@paulodamaso 0rat seems to have missed this PR, can you tell him to take it into scope? Thx :)

@paulodamaso
Copy link
Collaborator

@0crat in

@0crat
Copy link
Collaborator

0crat commented May 12, 2020

This pull request #476 is assigned to @vzurauskas/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

@vzurauskas
Copy link
Contributor

@paulodamaso The points listed in the PR description make sense, nothing to comment. Can be merged.

@paulodamaso
Copy link
Collaborator

@rultor merge

@rultor
Copy link
Collaborator

rultor commented May 12, 2020

@rultor merge

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

@rultor rultor merged commit 33820a0 into cqfn:master May 12, 2020
@rultor
Copy link
Collaborator

rultor commented May 12, 2020

@rultor merge

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

@victornoel victornoel deleted the 472 branch May 13, 2020 08:04
@vzurauskas
Copy link
Contributor

@0crat status

@0crat
Copy link
Collaborator

0crat commented May 15, 2020

@0crat status (here)

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

@vzurauskas
Copy link
Contributor

@paulodamaso I think this should be assigned to QA now?

@paulodamaso
Copy link
Collaborator

@0crat out

@0crat 0crat added qa and removed scope labels May 15, 2020
@0crat
Copy link
Collaborator

0crat commented May 15, 2020

@sereshqua/z please review this job completed by @vzurauskas/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

@0crat quality bad

@0crat 0crat added quality/bad and removed qa labels May 17, 2020
@0crat
Copy link
Collaborator

0crat commented May 17, 2020

Quality is low, no payment, see §31

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.

None yet

6 participants