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

#687 - Refactored RqForm into 1 interface and 3 smaller classes. #690

Merged
merged 11 commits into from
Jun 29, 2016

Conversation

rui-castro
Copy link
Contributor

@rui-castro rui-castro commented Jun 19, 2016

Issue #687: Removed checkstyle suppression ClassDataAbstractionCouplingCheck from RqForm, but is still needed in RqFormBase.

@davvd
Copy link

davvd commented Jun 20, 2016

@rui-castro Many thanks, I will find someone to review it before we merge

@davvd
Copy link

davvd commented Jun 20, 2016

@mkordas this pull request is for you, please review

@mkordas
Copy link

mkordas commented Jun 20, 2016

@rui-castro thanks for PR, I'll do a review tomorrow

@mkordas
Copy link

mkordas commented Jun 20, 2016

@rui-castro can you remove word closes from PR description? Only author of issue can close it, we cannot let GitHub do it.


/**
* Implementations of the interface {@link org.takes.rq.RqForm}.
*
Copy link

Choose a reason for hiding this comment

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

@rui-castro
Copy link
Contributor Author

@mkordas Removed closed from PR description and fixed comment formatting.

@mkordas
Copy link

mkordas commented Jun 21, 2016

@rui-castro thanks, I will continue my review soon

* @author Aleksey Popov (alopen@yandex.ru)
* @version $Id$
* @since 0.9
* @checkstyle ClassDataAbstractionCouplingCheck (500 lines)
Copy link

Choose a reason for hiding this comment

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

@rui-castro looks like issue is not fully fixed, we need to create additional puzzle for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkordas yes, I can add a todo but I don't know how to create a puzzle.

Copy link

Choose a reason for hiding this comment

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

@rui-castro just add @todo as in other places :)

@rui-castro
Copy link
Contributor Author

@mkordas I think I addressed all your comments with my last commit.

@mkordas
Copy link

mkordas commented Jun 22, 2016

@rui-castro I'll check it soon

}

/**
* RqFormBase returns always the same instances (Cache).
Copy link

Choose a reason for hiding this comment

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

@rui-castro second word should be can

@mkordas
Copy link

mkordas commented Jun 22, 2016

@rui-castro Travis build has failed

@mkordas
Copy link

mkordas commented Jun 22, 2016

@rui-castro see my review above

@mkordas
Copy link

mkordas commented Jun 23, 2016

@rui-castro can you please solve conflicts here?

@mkordas
Copy link

mkordas commented Jun 25, 2016

@rui-castro I'll check it soon

@mkordas
Copy link

mkordas commented Jun 25, 2016

@rui-castro Travis build is failed, we need to fix it

@mkordas
Copy link

mkordas commented Jun 25, 2016

@rui-castro I think that description of puzzle is still too short:

ERROR: "puzzle src/main/java/org/takes/rq/form/package-info.java:36-38 has a very short description of just 19 words while a minimum of 20 is required"

@mkordas
Copy link

mkordas commented Jun 25, 2016

@rui-castro see my comment above

@rui-castro
Copy link
Contributor Author

@mkordas the build is fixed.

@mkordas
Copy link

mkordas commented Jun 26, 2016

@rui-castro I'll check it in a sec :)

@mkordas
Copy link

mkordas commented Jun 26, 2016

@rui-castro sorry for a delay, on it now

@mkordas
Copy link

mkordas commented Jun 26, 2016

@rui-castro looks good!

@mkordas
Copy link

mkordas commented Jun 26, 2016

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Jun 26, 2016

@rultor merge

@mkordas Thanks for your request. @yegor256 Please confirm this.

@rui-castro
Copy link
Contributor Author

@mkordas thanks 😉

@mkordas
Copy link

mkordas commented Jun 28, 2016

@yegor256 we need your review here

@yegor256
Copy link
Owner

@rultor try to merge

@rultor
Copy link
Collaborator

rultor commented Jun 28, 2016

@rultor try to merge

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

@rultor rultor merged commit 5041d09 into yegor256:master Jun 29, 2016
@rultor
Copy link
Collaborator

rultor commented Jun 29, 2016

@rultor try to merge

@yegor256 Done! FYI, the full log is here (took me 37min)

@davvd
Copy link

davvd commented Jun 30, 2016

@ypshenychka please, review this issue for QA compliance, as per par.24

@davvd
Copy link

davvd commented Jun 30, 2016

@rultor please deploy

@rultor
Copy link
Collaborator

rultor commented Jun 30, 2016

@rultor please deploy

@davvd OK, I'll try to deploy now. You can check the progress here

@rultor
Copy link
Collaborator

rultor commented Jun 30, 2016

@rultor please deploy

@davvd Done! FYI, the full log is here (took me 33min)

@ypshenychka
Copy link

@rui-castro
According to our QA Rules:

Pull request description explains the solution proposed and contains a link to the original ticket it is related to.

Please provide a link to original ticket in PR's description.

@rui-castro rui-castro changed the title Refactored RqForm into 1 interface and 3 smaller classes. #687 - Refactored RqForm into 1 interface and 3 smaller classes. Jun 30, 2016
@rui-castro
Copy link
Contributor Author

@ypshenychka done

@ypshenychka
Copy link

@rui-castro Thank you.

@ypshenychka
Copy link

@davvd Quality is acceptable.

@davvd
Copy link

davvd commented Jul 1, 2016

@davvd Quality is acceptable.

@ypshenychka thank you, let's try to make it "good" next time

@davvd
Copy link

davvd commented Jul 1, 2016

@mkordas 10 mins was added to the account of @ypshenychka (for QA review), in transaction 91761879. Thank you, I have added 52 mins to your account in payment/transaction "577647ae80c2b22187000434", time consumed: 216 hours and 8 mins. you're getting extra minutes here (c=37). +52 added to your rating, at the moment it is: +6236

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