#780 Implement Condition.Range #801

t-izbassar · 2018-02-15T09:04:01Z

PR for #780

0crat · 2018-02-15T09:04:25Z

Job #801 is now in scope, role is REV

codecov-io · 2018-02-15T09:12:53Z

Codecov Report

Merging #801 into master will increase coverage by 0.04%.
The diff coverage is 92.3%.

@@             Coverage Diff              @@
##             master     #801      +/-   ##
============================================
+ Coverage     76.62%   76.66%   +0.04%     
  Complexity      953      953              
============================================
  Files           215      215              
  Lines          4517     4530      +13     
  Branches        359      361       +2     
============================================
+ Hits           3461     3473      +12     
  Misses          904      904              
- Partials        152      153       +1

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/takes/misc/Condition.java	`87.5% <92.3%> (+3.28%)`	`0 <0> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f355160...de35092. Read the comment docs.

0crat · 2018-02-15T09:14:37Z

This pull request #801 is assigned to @g4s8 /z, here is why. The budget is 15 minutes, see §4. Please, read §27 and when you decide to accept the changes, inform @yegor256 /z (the architect) right in this ticket. If you decide that this PR should not be accepted ever, also inform the architect.

g4s8 · 2018-02-15T10:04:10Z

@t-izbassar method fits() has a side-effect, it's not clear and may produce unpredictable behavior, because each call to this method will change internal state of an object.
Also I don't think that it's a correct design to implement Range as a Condition type (because range is not a condition logically) and use it inside Select:

new Select<Integer>(
  Arrays.asList(3, 5, 7, 9, 11, 13, 15),
  new Condition.Range<Integer>(2, 4)
); // -> [7, 9, 11]

In my opinion Range should be Iterable and used like this:

new Range<Integer>(
  Arrays.asList(3, 5, 7, 9, 11, 13, 15),
  2,
  4
); // -> [7, 9, 11]

@yegor256 what do you think? Maybe we can use https://github.com/yegor256/cactoos here?

t-izbassar · 2018-02-15T10:17:27Z

@g4s8 I agree about side-effect, but that's the only possible way to do this task. Range can be a condition because the condition is that the element fits given range. I don't see semantic misconception in that case. Also, this is what was requested by original issue: #281.

Your suggestion is valid. However, it cannot be used with Select and cannot be combined with other conditions. Example:

new Select<Integer>(
  Arrays.asList(3, 5, 7, 9, 11, 13, 15),
  new Condition.Not<Integer>(
    new Condition.Range<Integer>(2, 4)
  )
); // --> [3, 5, 13, 15]

Regarding adding cactoos - Takes has obligation to not bring any 3-rd party dependencies.

t-izbassar · 2018-02-15T10:35:46Z

@g4s8 and can you clarify, what exactly an unpredictable behavior is? When Range object is created, it's leave with the state, and state changes. That's seems natural to an object. It's still immutable. Also, the usage of conditions are limited to Select, which checks the boundaries within Iterable.

Of course, if very same object will be used in two different selects, behavior will be questionable. However, what is your take on this issue? I don't see how I can possibly address that. Maybe, just mentioning in javadoc of the class will be useful?

g4s8 · 2018-02-15T11:15:44Z

@t-izbassar

can you clarify, what exactly an unpredictable behavior is?

I mean that Range.fits expected to be used only in Select because Select knows that each call to fits() will change internal counter. If Range is used in other classes, it will cause some maintenance issues, because each class which use Range must know how it implemented internally.

requested by original issue: #281.

This Range implementation was mentioned in a comment #281 (comment) (not in a task description), this is why I'm aksing @yegor256 to look at this changes. If this comment is fully valid, I'd accept this PR.

t-izbassar · 2018-02-15T13:02:57Z

@g4s8 original task requested to have Condition.Skip. And skip is easy operation when you have Range. So this could be treated as being relevant part to what was requested originally. Also implementing Skip without Range will be harder. And it will have all the issues that you've already mentioned. It will have side-effect.

The whole purpose of having Condition is to use it in Select. The all test cases even placed in SelectTest. So I would not reject that as being coupled to Select usage - that's just design decision. If you think it is wrong, then you can submit issue or propose something else. But, with regards to implemented design, I believe this solution is valid.

t-izbassar · 2018-02-15T19:35:14Z

@yegor256 ping

yegor256 · 2018-02-16T15:48:18Z

@g4s8 looks like we are reinventing what Cactoos already has, you are right... Maybe we should just let Takes have one dependency on Cactoos?

t-izbassar · 2018-02-16T16:03:35Z

@yegor256 @g4s8 I can add this dependency in this PR and left puzzles to delete this Select and Condition staff. Will it be okay?

g4s8 · 2018-02-16T16:59:33Z

@t-izbassar I think you can keep this code, just add a @todo to replace Select and Condition with Cactoos analogs

t-izbassar · 2018-02-16T17:59:39Z

@g4s8 done

g4s8

@yegor256 I think it can be merged now

t-izbassar · 2018-02-17T12:37:04Z

@yegor256 ping

t-izbassar · 2018-02-19T09:39:48Z

@yegor256 ping

t-izbassar · 2018-02-19T17:55:36Z

@yegor256 can you merge this one?

yegor256 · 2018-02-19T19:19:25Z

@rultor merge

rultor · 2018-02-19T19:20:34Z

@rultor merge

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

rultor · 2018-02-19T19:25:57Z

@rultor merge

@t-izbassar @yegor256 Oops, I failed. You can see the full log here (spent 5min)

+ status=running
+ '[' running == null ']'
+ '[' running == success ']'
+ '[' running == failed ']'
+ echo 'waiting for AppVeyor build 2964: running'
waiting for AppVeyor build 2964: running
+ sleep 5s
+ true
++ curl -K ../curl-appveyor.cfg https://ci.appveyor.com/api/projects/yegor256/takes/build/2964
++ jq -r .build.status
+ status=running
+ '[' running == null ']'
+ '[' running == success ']'
+ '[' running == failed ']'
+ echo 'waiting for AppVeyor build 2964: running'
waiting for AppVeyor build 2964: running
+ sleep 5s
+ true
++ curl -K ../curl-appveyor.cfg https://ci.appveyor.com/api/projects/yegor256/takes/build/2964
++ jq -r .build.status
+ status=running
+ '[' running == null ']'
+ '[' running == success ']'
+ '[' running == failed ']'
+ echo 'waiting for AppVeyor build 2964: running'
waiting for AppVeyor build 2964: running
+ sleep 5s
+ true
++ curl -K ../curl-appveyor.cfg https://ci.appveyor.com/api/projects/yegor256/takes/build/2964
++ jq -r .build.status
+ status=running
+ '[' running == null ']'
+ '[' running == success ']'
+ '[' running == failed ']'
+ echo 'waiting for AppVeyor build 2964: running'
waiting for AppVeyor build 2964: running
+ sleep 5s
+ true
++ curl -K ../curl-appveyor.cfg https://ci.appveyor.com/api/projects/yegor256/takes/build/2964
++ jq -r .build.status
+ status=running
+ '[' running == null ']'
+ '[' running == success ']'
+ '[' running == failed ']'
+ echo 'waiting for AppVeyor build 2964: running'
waiting for AppVeyor build 2964: running
+ sleep 5s
+ true
++ curl -K ../curl-appveyor.cfg https://ci.appveyor.com/api/projects/yegor256/takes/build/2964
++ jq -r .build.status
+ status=failed
+ '[' failed == null ']'
+ '[' failed == success ']'
+ '[' failed == failed ']'
+ echo 'see https://ci.appveyor.com/project/yegor256/takes/build/2964'
see https://ci.appveyor.com/project/yegor256/takes/build/2964
+ exit 1
container 4fa33287c2712a2129cd0b2b6f54259864ea8b70acb7ee040db0e02a81c45f57 is dead
Mon Feb 19 20:25:51 CET 2018

0crat · 2018-02-20T11:56:58Z

@g4s8 /z this job was assigned to you 5 days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

t-izbassar · 2018-02-20T19:12:18Z

@yegor256 hi, can you re-trigger this merge as well? Thanks.

t-izbassar · 2018-02-21T17:49:31Z

@yegor256 ping for the merge

yegor256 · 2018-02-21T19:35:09Z

@rultor merge

rultor · 2018-02-21T19:36:38Z

@rultor merge

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

rultor · 2018-02-21T19:52:54Z

@rultor merge

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

0crat · 2018-02-21T19:53:45Z

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

elenavolokhova · 2018-02-22T08:21:05Z

@g4s8 According to our QA rules

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

Only two issues were found during review.
Please, confirm that you'll try to find at least three design problems in next reviews and will pay attention to correct names and descriptions.

Also,

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

This PR has no explanation of the solution proposed. As a code reviewer you should ask PR author to fix such issues too.

P.S. I'll need your confirmation on each ticket, where rules are not followed, in order to complete QA review and to motivate you to pay attention to these rules in future :)
Thanks!

elenavolokhova · 2018-02-22T08:22:02Z

@t-izbassar Please, add some explanation of the solution proposed to the description section.
Thanks!

t-izbassar · 2018-02-22T08:57:02Z

@elenavolokhova confirmed.

@t-izbassar Please, add some explanation of the solution proposed to the description section.

0crat · 2018-02-23T09:15:07Z

@g4s8 /z this job was assigned to you 8 days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

g4s8 · 2018-02-23T14:45:14Z

@elenavolokhova sure, I confirm

elenavolokhova · 2018-02-24T05:10:45Z

@0crat quality acceptable

0crat · 2018-02-24T05:12:30Z

@0crat quality acceptable (here)

@elenavolokhova There is an unrecoverable failure on my side. Please, submit it here:

com.zerocracy.farm.reactive.StkGroovy[125] java.lang.IllegalStateException: java.lang.reflect.InvocationTargetException in com/zerocracy/stk/pm/cost/make_payment.groovy
sun.reflect.NativeMethodAccessorImpl[-2] java.lang.reflect.InvocationTargetException: null
com.jcabi.xml.ListWrapper[162] com.jcabi.xml.ListWrapper$NodeNotFoundException: XPath '//*[@id='45.share']/text()' not found in '<?xml version="1.0" encoding="UTF-8"?><html xmlns=..35780..</script>\uA      </body>\uA   \uA</html>': Index (0) is out of bounds (size=0)

1.0-SNAPSHOT /cc @yegor256

0crat · 2018-02-24T05:13:54Z

QA review completed: +8 points just awarded to @elenavolokhova /z, total is +83

0crat · 2018-02-24T05:15:13Z

Order was finished, quality was "acceptable": +15 points just awarded to @g4s8 /z, total is +4615

0crat · 2018-02-24T05:15:26Z

@0crat quality acceptable (here)

@elenavolokhova You can't do that, unless you have one of these roles: ARC, PO. Your current roles are: QA.

0crat · 2018-02-24T05:15:48Z

Payment to ARC for a closed pull request, as in §28: +10 points just awarded to @yegor256 /z, total is +10824

0crat · 2018-02-26T20:16:59Z

This pull request #801 is assigned to @g4s8 /z, here is why. The budget is 15 minutes, see §4. Please, read §27 and when you decide to accept the changes, inform @yegor256 /z (the architect) right in this ticket. If you decide that this PR should not be accepted ever, also inform the architect.

g4s8 · 2018-03-03T11:13:31Z

@0crat status

0crat · 2018-03-03T11:13:41Z

@0crat status (here)

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

The job #801 is in scope for 16days
The role is REV
The job is assigned to @g4s8 /z for 5days
There is a monetary reward attached
@g4s8 /z will get 25% of their hourly rate for this job
The job doesn't have any impediments
The budget is 15 minutes/points
These users are banned and won't be assigned:
- @t-izbassar /z: This user reported the ticket
Job footprint (restricted area)

g4s8 · 2018-03-03T11:13:51Z

@yegor256 could you please remove this job from WBS?

g4s8 · 2018-03-06T20:05:20Z

@yegor256 ping

0crat · 2018-03-06T20:05:26Z

Are you speaking to me or about me here. You must always start your message with my name if you want to address it to me, see §1.

0crat · 2018-03-08T20:17:55Z

The user @g4s8 /z resigned from #801, please stop working. Reason for job resignation: It is older than 10 days, see §8

0crat · 2018-03-08T20:17:58Z

Resigned on delay, see §8: -15 points just awarded to @g4s8 /z

yegor256 · 2018-03-09T22:35:36Z

@0crat out

0crat · 2018-03-09T22:36:02Z

@0crat out (here)

@yegor256 The job #801 is now out of scope

yegor256#780 Implement Condition.Range

4810e9c

0crat added the scope label Feb 15, 2018

yegor256#780 Update todo description to bring Cactoos

de35092

g4s8 approved these changes Feb 16, 2018

View reviewed changes

t-izbassar mentioned this pull request Feb 19, 2018

RsXsltTest.loadsExternalImports failing #805

Closed

t-izbassar mentioned this pull request Feb 21, 2018

Condition.java:36-41: Implement Condition.Range that... #780

Closed

yegor256 mentioned this pull request Feb 21, 2018

Takes does not work well together with Cactoos #804

Closed

rultor merged commit de35092 into yegor256:master Feb 21, 2018

t-izbassar deleted the condition_range branch February 22, 2018 08:56

0crat removed the scope label Mar 9, 2018

#780 Implement Condition.Range #801

#780 Implement Condition.Range #801

Conversation

t-izbassar commented Feb 15, 2018

0crat commented Feb 15, 2018

codecov-io commented Feb 15, 2018 • edited Loading

Codecov Report

0crat commented Feb 15, 2018

g4s8 commented Feb 15, 2018

t-izbassar commented Feb 15, 2018 • edited Loading

t-izbassar commented Feb 15, 2018

g4s8 commented Feb 15, 2018

t-izbassar commented Feb 15, 2018

t-izbassar commented Feb 15, 2018

yegor256 commented Feb 16, 2018

t-izbassar commented Feb 16, 2018

g4s8 commented Feb 16, 2018

t-izbassar commented Feb 16, 2018

g4s8 left a comment

Choose a reason for hiding this comment

t-izbassar commented Feb 17, 2018

t-izbassar commented Feb 19, 2018

t-izbassar commented Feb 19, 2018

yegor256 commented Feb 19, 2018

rultor commented Feb 19, 2018

rultor commented Feb 19, 2018

0crat commented Feb 20, 2018

t-izbassar commented Feb 20, 2018

t-izbassar commented Feb 21, 2018

yegor256 commented Feb 21, 2018

rultor commented Feb 21, 2018

rultor commented Feb 21, 2018

0crat commented Feb 21, 2018

elenavolokhova commented Feb 22, 2018

elenavolokhova commented Feb 22, 2018

t-izbassar commented Feb 22, 2018

0crat commented Feb 23, 2018

g4s8 commented Feb 23, 2018

elenavolokhova commented Feb 24, 2018

0crat commented Feb 24, 2018

0crat commented Feb 24, 2018

0crat commented Feb 24, 2018

0crat commented Feb 24, 2018

0crat commented Feb 24, 2018

0crat commented Feb 26, 2018

g4s8 commented Mar 3, 2018

0crat commented Mar 3, 2018

g4s8 commented Mar 3, 2018

g4s8 commented Mar 6, 2018

0crat commented Mar 6, 2018

0crat commented Mar 8, 2018

0crat commented Mar 8, 2018

yegor256 commented Mar 9, 2018

0crat commented Mar 9, 2018

codecov-io commented Feb 15, 2018 •

edited

Loading

t-izbassar commented Feb 15, 2018 •

edited

Loading