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

shortcut for Optional.get named isPresent #113

Closed
robstoll opened this issue Jul 23, 2019 · 12 comments
Closed

shortcut for Optional.get named isPresent #113

robstoll opened this issue Jul 23, 2019 · 12 comments
Assignees
Labels
Milestone

Comments

@robstoll
Copy link
Owner

robstoll commented Jul 23, 2019

Platform (JVM and/or JS): JVM (only jdk8)

Code related feature

expect(Optional.of(1)).isPresent().toBe(1)                  // isPresent incorporates check for empty
//instead of
expect(Optional.of(1)).feature(Optional::get).toBe(1) // get throws in case of empty

//and
expect(Optional.of(1)).isPresent {  // isPresent incorporates check for empty
  isGreaterThan(1)
  isLessThan(4)
}
//instead of 
expect(Optional.of(1)).feature(Optional::get) { // get throws in case of empty
  isGreaterThan(1)
  isLessThan(4)
}

In reporting we want to see for

expect(Optional.empty<Int>).isPresent().toBe(1)

the following report:

expect: Optional.empty
◆ ▶ get(): ❗❗is empty

and for

expect(Optional.empty<Int>).isPresent() { toBe(1) }

the following

expect: Optional.empty
◆ ▶ get(): ❗❗is empty
  >> to be: 1

and for

expect(Optional.of(2)).isPresent() { toBe(1) }

the following:

expect: Optional[2]
◆ ▶ get(): 2
   ◾ to be: 1

Following the things you need to do:

lib

  • implement _isPresent in optionalAssertions by using the `ExpectImpl.feature.extractor... (see resultAssertions.kt _isSuccess as guideline

domain

  • extend OptionalAssertions with a function isPresent which returns ExtractedFeaturePostStep<T, E> where T: Optional<E> (see ResultAssertions.isSucces as a guideline)
  • modify OptionalAssertionsBuilder, delegate to optionalAssertions (see ListAssertionsBuilder as a guideline)
  • delegate implementation to robstoll-lib in OptionalAssertionsImpl (see ListAssertionsImpl as a guideline)

api

  • provide a val which returns Expect<E> (see resultAssertions.kt as a guideline) and add @since 0.9.0 to the KDOC
  • provide a fun which expects an assertionCreator lambda and returns Expect<T> (see resultAssertions.kt as a guideline) and @since 0.9.0 (adopt to current version) to the KDOC
  • extend OptionalFeatureAssertionsSpec in specs-common (see for instance ListFeatureAssertionsSpec) and extend it in atrium-api-fluent-en_GB-common/src/test

Your first contribution?

  • Write a comment I'll work on this if you would like to take this issue over.
    This way we get the chance to revise the description in case things have changed in the meantime,
    we might give you additional hints and we can assign the task to you, so that others do not start as well.
  • See Your first code contribution for guidelines.
  • Do not hesitate to ask questions here or to contact us via Atrium's slack channel if you need help
    (Invite yourself in case you do not have an account yet).
@jGleitz
Copy link
Collaborator

jGleitz commented Aug 21, 2019

Suggestion: if get would be called toBePresent (without any other change), it would be more obvious that it actually contains an assertion.

@robstoll
Copy link
Owner Author

I try to use the same names one is already familiar with for feature extraction, IMO this way we have a more intuitive API than inventing new names. We already have List.get, Throwable.message let's stick with that pattern. We could introduce isPresent in addition though.

@robstoll
Copy link
Owner Author

robstoll commented Oct 1, 2019

I mulled your input over and I came to the conclusion that get might indeed not be the best choice for the naming. I think isPresent is better as it is an obvious counterpart to isEmpty and many people argue that it is discouraged to use Optional.get (because it throws if empty). Thus I guess those people would look for an isPresent or getOrElse but getOrElse does not make sense in this context.

@robstoll robstoll changed the title shortcut for Optional.get shortcut for Optional.get named isPresent Oct 4, 2019
@byarr
Copy link

byarr commented Oct 8, 2019

I'll work on this

@jGleitz
Copy link
Collaborator

jGleitz commented Oct 9, 2019

I think isPresent is better than get. And I think you give good reasons!

@robstoll
Copy link
Owner Author

@byarr I have updated the description regarding reporting.

@robstoll
Copy link
Owner Author

@byarr do you need help?

@byarr
Copy link

byarr commented Oct 27, 2019

@robstoll Sorry, I did take quick look at this. But I havnt really got the time right now. Unassigning myself.

@byarr byarr removed their assignment Oct 27, 2019
@robstoll
Copy link
Owner Author

@byarr no problem if you need longer, just wanted to make sure you are not stuck. Rich out here if you want to take it over again and I re-assign it to you.

@slalu
Copy link

slalu commented Jan 15, 2020

I'll work on this.

In the description, you mention "ExtractedFeatureOption" but I don't find this class. I assumed it was ExtractedFeaturePostStep when I looked at the examples provided. Am I right or I am missing something?

@robstoll
Copy link
Owner Author

That's correct. The class was renamed since I created this issue

robstoll added a commit that referenced this issue Jan 31, 2020
@robstoll
Copy link
Owner Author

implemented with 8cd76f3

@robstoll robstoll added this to the 0.9.0 milestone Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants