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.

Sign up for GitHub

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

Jump to bottom

ADD: samples of iterableSubjectChangers in api-infix and api-fluent #1155

Merged
merged 6 commits into from
Jun 18, 2022
Merged

ADD: samples of iterableSubjectChangers in api-infix and api-fluent #1155

merged 6 commits into from
Jun 18, 2022

Conversation

simonNozaki
Copy link
Contributor

Preview

Adding samples of iterableSubjectChangers to api-infix and api-fluent.

Related Issue

#1038

I confirm that I have read the Contributor Agreements v1.0, agree to be bound on them and confirm that my contribution is compliant.

robstoll
robstoll requested changes Jun 16, 2022
View reviewed changes
Copy link
Owner

@robstoll robstoll left a comment

Choose a reason for hiding this comment

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

@simonNozaki see ArraySubjectChangerSamples, please copy the contents from there regarding subject changes etc.

@simonNozaki
Copy link
Contributor Author

@simonNozaki see ArraySubjectChangerSamples, please copy the contents from there regarding subject changes etc.

@robstoll ,
Sorry, I see. I have a question. ArraySubjectChangerSamples has many methods except asList .Should I copy those methods?

@robstoll
Copy link
Owner

Only the first two are relevant for Iterable

@simonNozaki
Copy link
Contributor Author

simonNozaki commented Jun 16, 2022 •
edited
Loading

@robstoll

I understand IterableSubjectChangersSamples will transform subject of classes inherited from Iterable, so for example, asListFeature expectations will be like this, right?

fun asListFeature() {
    expect('A'..'B') asList o toEqual listOf('A', 'B')
        //                           | subject is now of type List<Char>
}

iterableSubjectChangers provides transformers from Iterable like CharRange, so I thought subject of expectations should not be the inheritance of Collection like Array, List and so on.

@robstoll
Copy link
Owner

@simonNozaki using a Range as you did is perfectly fine. It would indeed not make sense to use List or a subtype of List because asList exactly turns the subject into a List.
However, you could have used any other Iterable/Collection type such as Set instead of Range but as said, Range is perfectly fine. Keep it this way. What I miss are the comments about what is going on. I have left two review comments above, so that you better understand what I mean (see above I already left a comment about renaming IterableSubjectChangersSamples to IterableSubjectChangerSamples).
Do you see what is missing?

@simonNozaki
Copy link
Contributor Author

using a Range as you did is perfectly fine.
Thanks!

What I miss are the comments about what is going on.
Oh, comments... Renaming classes and adding comments to samples, so review again, please?

robstoll
robstoll requested changes Jun 17, 2022
View reviewed changes
...ommonTest/kotlin/ch/tutteli/atrium/api/fluent/en_GB/samples/IterableSubjectChangerSamples.kt Outdated Show resolved Hide resolved
...ommonTest/kotlin/ch/tutteli/atrium/api/fluent/en_GB/samples/IterableSubjectChangerSamples.kt Outdated Show resolved Hide resolved
...commonTest/kotlin/ch/tutteli/atrium/api/infix/en_GB/samples/IterableSubjectChangerSamples.kt Outdated Show resolved Hide resolved
...commonTest/kotlin/ch/tutteli/atrium/api/infix/en_GB/samples/IterableSubjectChangerSamples.kt Outdated
.asList {
it toContain 3 // fails
it toContain 4 // still evaluated even though above `toContain` already fails
// use `asList o` if you want a fail fast behaviour
Copy link
Owner

Choose a reason for hiding this comment

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

Align

Copy link
Owner

Choose a reason for hiding this comment

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

The problem are the parameter names in intellij. You could hide them but I am going to fix the alignment, that's fine.

Copy link
Owner

Choose a reason for hiding this comment

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

I see you fixed it already 🙂👍

...ommonTest/kotlin/ch/tutteli/atrium/api/fluent/en_GB/samples/IterableSubjectChangerSamples.kt

@Test
fun asList() {
expect(0..2) // subject within this expectation-group is of type List<Int>
Copy link
Owner

Choose a reason for hiding this comment

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

Move comment one line below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

robstoll
robstoll requested changes Jun 17, 2022
View reviewed changes
...commonTest/kotlin/ch/tutteli/atrium/api/infix/en_GB/samples/IterableSubjectChangerSamples.kt Outdated
.asList {
it toContain 3 // fails
it toContain 4 // still evaluated even though above `toContain` already fails
// use `asList o` if you want a fail fast behaviour
Copy link
Owner

Choose a reason for hiding this comment

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

I see you fixed it already 🙂👍

...ommonTest/kotlin/ch/tutteli/atrium/api/fluent/en_GB/samples/IterableSubjectChangerSamples.kt Outdated
@Test
fun asList() {
expect(0..2)
// subject within this expectation-group is of type List<Int>
Copy link
Owner

Choose a reason for hiding this comment

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

This one is still on the wrong line. Did you forget to push your changes?

Copy link
Contributor Author

@simonNozaki simonNozaki Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@robstoll

Move comment one line below

I saw such a comment, so I moved this comment one line below(Before chainging, this comment was on the line with expect(0..2))

Copy link
Owner

@robstoll robstoll Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I see, a misunderstanding. Put it after asList (like you did it api-infix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robstoll
Fixed this.

@simonNozaki
Copy link
Contributor Author

@robstoll ,

As far as I can, I fixed alignment of spaces. Would you review again, please?

@robstoll
Copy link
Owner

@simonNozaki great, thanks for your first contribution to Atrium 🙂👍

@simonNozaki
Copy link
Contributor Author

@robstoll

Thank you for your kind support! This is my first contribution ever to open source projects! I want to get more experience for this repository, so would you assign me again other issues like #1129, #1046, and so on?(I'm interested in #1129)

@robstoll robstoll merged commit 0797d3b into robstoll:main Jun 18, 2022
@simonNozaki simonNozaki deleted the feature/add-iterableSubjectChangers-samples branch June 18, 2022 10:50
@robstoll robstoll modified the milestone: 0.19.0 Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robstoll robstoll robstoll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@simonNozaki @robstoll