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

For #452: Added LCOM4 report #458

Merged
merged 2 commits into from
Apr 22, 2020
Merged

For #452: Added LCOM4 report #458

merged 2 commits into from
Apr 22, 2020

Conversation

paulodamaso
Copy link
Collaborator

For #452:

  • Added LCOM4 report to Main.java
  • Left puzzle to document better jPeek usage on README.md

@0crat
Copy link
Collaborator

0crat commented Apr 21, 2020

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

Copy link

@Vatavuk Vatavuk left a comment

Choose a reason for hiding this comment

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

@paulodamaso one comment

@@ -200,6 +200,14 @@ public void analyze() throws IOException {
)
);
}
if (this.params.containsKey("LCOM4")) {
Copy link

Choose a reason for hiding this comment

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

@paulodamaso analyze is a really long method. How about we extract reports building to a separate class and use a Map instead of if statements? In that sense we can localize future changes in that class not in the main method.

@paulodamaso
Copy link
Collaborator Author

paulodamaso commented Apr 22, 2020

@Vatavuk Yes, you are right, I've added a puzzle for that, please take a look

Copy link

@Vatavuk Vatavuk left a comment

Choose a reason for hiding this comment

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

@paulodamaso thx, good to merge

@paulodamaso
Copy link
Collaborator Author

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Apr 22, 2020

@rultor merge

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

@rultor rultor merged commit ab8826a into cqfn:master Apr 22, 2020
@rultor
Copy link
Collaborator

rultor commented Apr 22, 2020

@rultor merge

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

@paulodamaso paulodamaso deleted the 452 branch April 23, 2020 02:42
@Vatavuk
Copy link

Vatavuk commented Apr 23, 2020

@paulodamaso please remove this issue from the scope

@paulodamaso
Copy link
Collaborator Author

@0crat out

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

0crat commented Apr 23, 2020

Job was finished in 47 hours, bonus for fast delivery is possible (see §36)

@0crat
Copy link
Collaborator

0crat commented Apr 23, 2020

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

@0crat 0crat removed the scope label Apr 23, 2020
@sereshqua
Copy link

@Vatavuk please make sure you will find at least 3 issues during next CR, thanks

@Vatavuk
Copy link

Vatavuk commented Apr 23, 2020

@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.

5 participants