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 #72: Introduce error mechanics. #75

Merged
merged 2 commits into from
Oct 26, 2018
Merged

Conversation

paulodamaso
Copy link
Contributor

For #72: Introduce error mechanics:

  • Added Error and Errors interfaces
  • Changed User to return user errors
  • Created tests for DyError and DyErrors implementations
  • Left puzzles for continue implementation

@paulodamaso paulodamaso mentioned this pull request Oct 23, 2018
@0crat 0crat added the scope label Oct 23, 2018
@0crat
Copy link
Collaborator

0crat commented Oct 23, 2018

Job #75 is now in scope, role is REV

@0crat
Copy link
Collaborator

0crat commented Oct 23, 2018

This pull request #75 is assigned to @pinaf/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @carlosmiranda/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

@pinaf pinaf 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 see my comments below.

* @author Paulo Lobo ([email protected])
* @version $Id$
* @since 1.0
* @todo #72:30min Implement DyError and DyErrors methods. These
Copy link

Choose a reason for hiding this comment

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

@paulodamaso I believe you mean classes here instead of methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pinaf Actually I was meaning "methods from DyError and DyErrors", but I'll stick to your suggestion since it describes the issue better.

* Delete it.
* @throws IOException If fails
*/
void delete() throws IOException;
Copy link

Choose a reason for hiding this comment

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

@paulodamaso why do we want to delete errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pinaf After we see the error on error page it would be deleted, just like en Event

@@ -54,6 +54,9 @@
* @author Yegor Bugayenko ([email protected])
* @version $Id$
* @since 0.13
* @todo #72:30min Errors occurred during execution must be saved and later
* showed to user. Wire Error and Errors implementations in Exec flow so these
* errors are properly logged once they happen. Cover this with tests.
Copy link

Choose a reason for hiding this comment

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

@paulodamaso how about a puzzle for listing the errors and displaying them somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pinaf That's exactly what this puzzle is about. Let me reword it to become more clear.

@paulodamaso
Copy link
Contributor Author

@pinaf Made some changes, please take a look now.

@pinaf
Copy link

pinaf commented Oct 26, 2018

@paulodamaso thanks

@pinaf
Copy link

pinaf commented Oct 26, 2018

@carlosmiranda good to merge

@carlosmiranda
Copy link
Collaborator

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Oct 26, 2018

@rultor merge

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

@rultor rultor merged commit 5cbeaf9 into yegor256:master Oct 26, 2018
@rultor
Copy link
Collaborator

rultor commented Oct 26, 2018

@rultor merge

@carlosmiranda Done! FYI, the full log is here (took me 14min)

@0crat
Copy link
Collaborator

0crat commented Oct 26, 2018

@ypshenychka/z please review this job completed by @pinaf/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 Oct 26, 2018
@0crat
Copy link
Collaborator

0crat commented Oct 26, 2018

The job #75 is now out of scope

@0crat
Copy link
Collaborator

0crat commented Oct 26, 2018

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @carlosmiranda/z

@ypshenychka
Copy link

@pinaf According to our QA Rules:

The code reviewer found at least three problems in the code.
Comments were mostly about design problems, not cosmetic issues.

Only two issues were found during code review.
Please confirm that you'll try to find at least three major problems while future reviews.

@pinaf
Copy link

pinaf commented Oct 29, 2018

@ypshenychka I confirm.

@ypshenychka
Copy link

@pinaf thanks

@ypshenychka
Copy link

@0crat quality acceptable

@ypshenychka
Copy link

@0crat quality acceptable

@0crat
Copy link
Collaborator

0crat commented Oct 30, 2018

@0crat quality acceptable (here)

@ypshenychka There is no quality review for #75, no performer

@0crat
Copy link
Collaborator

0crat commented Oct 30, 2018

Order was finished, quality is "acceptable": +15 point(s) just awarded to @pinaf/z

@0crat
Copy link
Collaborator

0crat commented Oct 30, 2018

Quality review completed: +8 point(s) just awarded to @ypshenychka/z

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

Successfully merging this pull request may close these issues.

6 participants