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

#589: Make the constructors of FkHitRefresh compliant with ConstructorOnlyInitializesOrCallOtherConstructors #593

Merged
merged 4 commits into from
Feb 24, 2016

Conversation

essobedo
Copy link
Contributor

Fix for #589

The goal of this PR is to make the class FkHitRefresh compliant with the rule ConstructorOnlyInitializesOrCallOtherConstructors by delegating the construction of the file to touch to a private static method.

@davvd
Copy link

davvd commented Feb 11, 2016

@essobedo Thanks for the pull request, let me find a reviewer..

@davvd
Copy link

davvd commented Feb 11, 2016

@longtimeago review this one, please

@@ -172,7 +164,16 @@ private boolean expired() {
* @throws IOException If fails
*/
private void touch() throws IOException {

Choose a reason for hiding this comment

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

@essobedo looks like we don't need this method anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@longtimeago Actually it is still needed as you can see here

Choose a reason for hiding this comment

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

@essobedo can't we pass this.last into private static void touch ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@longtimeago as you like

@longtimeago
Copy link

@essobedo see one remark above, thanks!

@longtimeago
Copy link

@essobedo much better now :)
Thanks!

@longtimeago
Copy link

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Feb 11, 2016

@rultor merge

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

@essobedo
Copy link
Contributor Author

@longtimeago thank you for the review

@longtimeago
Copy link

@essobedo you are welcome :)

@essobedo
Copy link
Contributor Author

@yegor256 please confirm

@essobedo
Copy link
Contributor Author

@yegor256 please confirm

@essobedo
Copy link
Contributor Author

@yegor256 This PR is waiting for a confirmation for more than a week now, so could you please do something about it? confirm it or reject it but please do something about it.

* @return The file to touch
* @throws IOException If fails
*/
private static File createFileToTouch() throws IOException {
Copy link
Owner

Choose a reason for hiding this comment

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

@yegor256
Copy link
Owner

@essobedo one comment above

@essobedo
Copy link
Contributor Author

@yegor256 thank you for the review and the remark. Please have a look to the new version of the PR. Thank you in advance.

@essobedo
Copy link
Contributor Author

@yegor256 please review the PR

1 similar comment
@essobedo
Copy link
Contributor Author

@yegor256 please review the PR

@yegor256
Copy link
Owner

@rultor try to merge

@rultor
Copy link
Collaborator

rultor commented Feb 24, 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 b132b22 into yegor256:master Feb 24, 2016
@essobedo
Copy link
Contributor Author

@yegor256 thx

@rultor
Copy link
Collaborator

rultor commented Feb 24, 2016

@rultor try to merge

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

@essobedo essobedo deleted the 589 branch February 24, 2016 16:44
@davvd
Copy link

davvd commented Feb 28, 2016

@elenavolokhova please, help us to evaluate quality here, as per par.24

@davvd
Copy link

davvd commented Feb 28, 2016

@rultor please deploy

@rultor
Copy link
Collaborator

rultor commented Feb 28, 2016

@rultor please deploy

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

@rultor
Copy link
Collaborator

rultor commented Feb 28, 2016

@rultor please deploy

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

@elenavolokhova
Copy link

@longtimeago
According to our quality rules:

The code reviewer found at least three problems in the code.

Only one issue was found during review.
Please confirm that you will try to find at least three problems next time.

@longtimeago
Copy link

@elenavolokhova I confirm

@elenavolokhova
Copy link

@davvd Quality is acceptable here.

@davvd
Copy link

davvd commented Feb 28, 2016

@davvd Quality is acceptable here.

@elenavolokhova thanks for the QA review, we'll work better next time

@davvd
Copy link

davvd commented Mar 2, 2016

@longtimeago paied 10 mins to @elenavolokhova for QA review (payment ID is 79180755); Much appreciated! 22 mins was added to your account, payment ID 79180778, time spent is 382 hours and 14 mins; you're getting extra minutes here (c=7); +22 to your rating, your total score is +6596

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