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

Implement Traversable#reject #2157

Merged
merged 3 commits into from
Nov 14, 2017

Conversation

valery1707
Copy link
Contributor

Fix #2156.

P.S.
A also replace Map#removeAll, Map#removeKeys and Map#removeValues with reject* variant. With deprecation of course.

@valery1707
Copy link
Contributor Author

JDK9 build failed with Missing dependency 'object java.lang.Object in compiler mirror', required by /home/travis/.m2/repository/org/scala-lang/scala-library/2.10.5/scala-library-2.10.5.jar(scala/package.class)

@ruslansennov ruslansennov added this to the vavr-1.0.0 milestone Nov 8, 2017
public M removeKeys(Predicate<? super K> predicate) {
Objects.requireNonNull(predicate, "predicate is null");
Copy link
Member

Choose a reason for hiding this comment

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

I believe you should save this checking. This will be more concrete if you will throw an exception in the called method but not in its implementation. This also applies to other deprecated methods. There are many methods in Vavr where this rule is not respected, but I believe that it should

@valery1707
Copy link
Contributor Author

@ruslansennov fixed.
Maybe move to milestone vavr-0.9.2?

@ruslansennov
Copy link
Member

fixed.

thank you!

Maybe move to milestone vavr-0.9.2?

this PR adds new methods to the Traversable interface, so it can't be included to the minor release

@codecov-io
Copy link

codecov-io commented Nov 13, 2017

Codecov Report

Merging #2157 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2157      +/-   ##
============================================
+ Coverage     97.45%   97.46%   +0.01%     
- Complexity     5206     5238      +32     
============================================
  Files            92       92              
  Lines         11934    11997      +63     
  Branches       1576     1578       +2     
============================================
+ Hits          11630    11693      +63     
  Misses          148      148              
  Partials        156      156
Impacted Files Coverage Δ Complexity Δ
.../src/main/java/io/vavr/collection/Collections.java 94.44% <ø> (ø) 116 <0> (ø) ⬇️
...r/src/main/java/io/vavr/collection/IndexedSeq.java 97.39% <ø> (ø) 37 <0> (ø) ⬇️
...vr/src/main/java/io/vavr/collection/SortedMap.java 100% <ø> (ø) 3 <0> (ø) ⬇️
...c/main/java/io/vavr/collection/SortedMultimap.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...avr/src/main/java/io/vavr/collection/Multimap.java 98.38% <ø> (ø) 28 <0> (ø) ⬇️
.../src/main/java/io/vavr/collection/Traversable.java 98.38% <ø> (ø) 101 <0> (ø) ⬇️
vavr/src/main/java/io/vavr/collection/Map.java 100% <ø> (ø) 34 <0> (ø) ⬇️
...vr/src/main/java/io/vavr/collection/LinearSeq.java 96.7% <ø> (ø) 20 <0> (ø) ⬇️
vavr/src/main/java/io/vavr/collection/Seq.java 98% <ø> (ø) 50 <0> (ø) ⬇️
vavr/src/main/java/io/vavr/collection/Set.java 66.66% <ø> (ø) 2 <0> (ø) ⬇️
... and 18 more

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 3365fd2...4c8e93b. Read the comment docs.

Copy link
Contributor

@danieldietrich danieldietrich left a comment

Choose a reason for hiding this comment

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

Clean PR!
It shows it isn't you first PR 😄
Many thanks for you ongoing efforts!

@@ -164,9 +164,16 @@ public Q removeAll(Iterable<? extends T> elements) {
return Collections.removeAll((Q) this, elements);
}

@SuppressWarnings("unchecked")
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

It is good to mark it! Maybe we can remove the method for Vavr 1.0.0 - but for now it can stay as-is. Thx!

@@ -58,11 +58,11 @@
* <li>{@link #filter(BiPredicate)}</li>
* <li>{@link #filterKeys(Predicate)}</li>
* <li>{@link #filterValues(Predicate)}</li>
* <li>{@link #reject(BiPredicate)}</li>
* <li>{@link #rejectKeys(Predicate)}</li>
* <li>{@link #rejectValues(Predicate)}</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

great, thx!

@danieldietrich danieldietrich merged commit 4273847 into vavr-io:master Nov 14, 2017
@valery1707 valery1707 deleted the traversable-reject branch November 15, 2017 07:06
danieldietrich pushed a commit that referenced this pull request Jan 13, 2019
* Implement Traversable#reject

* Replace Seq#removeAll(Predicate) with Traversable#reject(Predicate)

* Remove `reject` implementation from Queue and PriorityQueue
danieldietrich pushed a commit that referenced this pull request Jan 13, 2019
* Implement Traversable#reject

* Replace Seq#removeAll(Predicate) with Traversable#reject(Predicate)

* Remove `reject` implementation from Queue and PriorityQueue
danieldietrich pushed a commit that referenced this pull request Jan 16, 2019
* Implement Traversable#reject

* Replace Seq#removeAll(Predicate) with Traversable#reject(Predicate)

* Remove `reject` implementation from Queue and PriorityQueue
@danieldietrich danieldietrich mentioned this pull request Jan 19, 2019
15 tasks
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.

4 participants