Comments (12)
sure
from assertj.
Quite honestly, I had not even thought about that until you asked. I think I would be fine with both. The question of which one would be more nice for setting a breakpoint afterwards also depends, on whether I as the developer consider the first duplicate wrong (e.g. the first Merry should not have been added to begin with), or the second one. More typical might be to consider the second duplicate wrong, so I can understand that you would eventually prefer option 2.
As I said, I would be fine with either variant.
from assertj.
@Bananeweizen can I take this for my first contribution, would help me in getting familiar with the PR process
from assertj.
@kulgan good point, I agree with @Bananeweizen that there isn't much difference between the two options, so I would go with the simplest implementation on this one
from assertj.
Go for it @ranjitshinde91, thanks!
from assertj.
Fair enough, if I understand this correctly the fix should consist of using a linked hash set to store the duplicates.
@Bananeweizen do you want to contribute this one?
from assertj.
Yes, I can do that. I still have to understand if the currently used comparator has some useful meaning for another (unknown to me) aspect. If not, I'd also expect this to be a trivial replacement of the TreeSet by a LinkedHashSet.
from assertj.
If memory serves, I think we used a treeset to have consistent ordering in our tests, this can be achieved better with a linked hash set though.
from assertj.
TreeSet with custom comparator is used to compare objects in Set collections. Hence simply changing TreeSet to LinkedHashSet does not work.
Added fix as part of this PR #3333
from assertj.
@Bananeweizen quick question regarding the expected ordering. Given the following input which is found in the test ["Merry", "Frodo", null, null, "Merry", "Sam", "Frodo"]
, there are two possible ways to return the duplicates in order:
["Merry", "Frodo", null]
[null, "Merry", "Frodo"]
Option 1 starts with the first elements that is a duplicate, while the second starts with the first duplicated element. I think they are both valid in their own way, like for debugging purpose, option 2 tells me null is the first duplicated element, which I think is helpful, while option one seems like the natural way.
I know @ranjitshinde91 implementation is based on 1. above, checking if option 2 can be considered.
from assertj.
I agree with @Bananeweizen when I started implementation i considered second Merry to be wrong and added in duplicates.
Also 2nd Implementation is simpler and does not require second traversal of Interable done in private method.
I will make the necessary changes to switch to option 2, if that's okay ?
from assertj.
updated the same PR with these changes
from assertj.
Related Issues (20)
- Recursive comparison depends on the sequence of objects. HOT 4
- Recursive comparison checks for existence of fields in types that parameterize nested unordered iterables HOT 1
- `satisfiesExactlyInAnyOrder` fails if `actual` overrides `equals` HOT 4
- `isNotEqualTo(boolean)` fails if `actual` is null HOT 2
- Suggesting "hasOnlyDuplicates()" assertion HOT 3
- Field existence check for the elements of a collection field HOT 1
- Checking compared fields existence: handle containers
- ...usingRecursiveComparison().ignoringTransientFields()... HOT 7
- Unable to compare some timestamp types HOT 3
- Add AbstractAssert#matchesWith HOT 1
- AssertFactory with the ability to convert the value upfront HOT 24
- Add default recursive comparison configuration for non accessible classes and fields HOT 6
- `extracting(Function...)` propagates `Object` instead of the common base type of all extractors HOT 6
- NPE with `extracting(Function... extractors)` HOT 3
- AssertJ: issue with Assertions.assertThat().usingRecursiveComparison().isEqualTo() HOT 6
- Provide reason and possible replacement for @Deprecated HOT 3
- AssertJ string assertions not working with Spock 2.3 and Groovy HOT 15
- isEqualTo on date can lead to unexpected failures when switching time zones HOT 6
- `InstanceOfAssertFactories.array` and `InstanceOfAssertFactories.array2D` expect array type parameter instead of the one for their elements HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from assertj.