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

[wip] Make Context API non nullable #780

Merged
merged 4 commits into from
Aug 28, 2017
Merged

Conversation

smaldini
Copy link
Contributor

@smaldini smaldini commented Aug 3, 2017

  • add hasKey : boolean
  • add getOrEmpty : Optional

@smaldini smaldini added the type/enhancement A general enhancement label Aug 3, 2017
@smaldini smaldini added this to the 3.1.0.RC1 milestone Aug 3, 2017
@codecov-io
Copy link

codecov-io commented Aug 3, 2017

Codecov Report

Merging #780 into master will increase coverage by 0.35%.
The diff coverage is 95.91%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #780      +/-   ##
============================================
+ Coverage      82.5%   82.85%   +0.35%     
- Complexity     3026     3145     +119     
============================================
  Files           312      314       +2     
  Lines         25439    25710     +271     
  Branches       4752     4831      +79     
============================================
+ Hits          20989    21303     +314     
+ Misses         2953     2928      -25     
+ Partials       1497     1479      -18
Impacted Files Coverage Δ Complexity Δ
.../reactor/core/publisher/MonoSubscriberContext.java 70% <ø> (ø) 2 <0> (?)
...src/main/java/reactor/core/publisher/FluxSink.java 100% <ø> (ø) 0 <0> (ø) ⬇️
...e/src/main/java/reactor/util/context/Context5.java 100% <100%> (ø) 21 <21> (?)
...e/src/main/java/reactor/util/context/ContextN.java 100% <100%> (+66.66%) 14 <9> (+11) ⬆️
...e/src/main/java/reactor/util/context/Context3.java 100% <100%> (ø) 15 <15> (?)
...e/src/main/java/reactor/util/context/Context0.java 100% <100%> (+55.55%) 7 <2> (+4) ⬆️
...e/src/main/java/reactor/util/context/Context4.java 100% <100%> (ø) 18 <18> (?)
...src/main/java/reactor/core/publisher/FluxPeek.java 92.68% <100%> (+0.44%) 12 <0> (ø) ⬇️
...ore/src/main/java/reactor/core/publisher/Mono.java 95.04% <100%> (-0.02%) 226 <1> (-1)
...ore/src/main/java/reactor/core/publisher/Flux.java 100% <100%> (ø) 465 <1> (-1) ⬇️
... and 31 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 1d666fe...bac409c. Read the comment docs.

@smaldini smaldini changed the title Make Context API non nullable [wip] Make Context API non nullable Aug 3, 2017
Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

2 concerns with that approach:

  • is throwing on a get with bad key a good idea at all? think about the usage patterns and the try/catch or if(hasKey) this implies? granted there is a getOrEmpty but... I think I'd leave get as Nullable.
  • why remove getOrDefault at all? It is useful IMO, as much as getOrEmpty...

If you decide to keep the exceptions, at least document them in the javadocs ⚠️

@smaldini
Copy link
Contributor Author

smaldini commented Aug 4, 2017

It's wip but the point here is a get(...) that would return null would do so indefinitely. Context is immutable, a null key will never turn non null for a given ctx. User can still query presence of a key with hasKey or getOrEmpty (which superseed getOrDefault via Optionsl.orElse),

@simonbasle
Copy link
Member

got it, wasn't sure what would be the most complicated to use between a nullable get or a throwing get... but in any case it should be marked as less than ideal, maybe even directly deprecated?

fair point on getOrEmpty().orElse(), just thinking it just might be a bit more readable as a getOrDefault() alias / syntactic sugar?

@simonbasle simonbasle dismissed their stale review August 17, 2017 07:46

getOrDefault re-added

@simonbasle
Copy link
Member

simonbasle commented Aug 21, 2017

@smaldini was thinking about:

  • adding Context1-Context5 implementations (hidden, instead of the map one)
  • means of creating a Context1 directly
  • maybe the same for sizes up to 5, but what of the key-value parameters (have a look at how java is planning on doing that for sequence literals?)
  • add putAll

@simonbasle simonbasle self-assigned this Aug 22, 2017
@simonbasle simonbasle added the wip label Aug 22, 2017
@simonbasle
Copy link
Member

note on putAll: since this would return a Context, it clashes with Map#putAll in the ContextN implementation. I'm going with putAllOf but if you have a better suggestion @smaldini ...

Stephane Maldini and others added 4 commits August 24, 2017 11:54
 - rename contextStart to subscriberContext
 - remove contextGet in favor of existing handle
 - make most of Context API non nullable
 - add hasKey : boolean
 - add getOrEmpty : Optional<T>
 - add Context.of static factory methods for cardinalities 1-5
 - add specific implementations for cardinalities 1-5
 - add Context#putAll(Context)
 - fix isEmpty to also detect (internal only) instances of Context0
 - add tests for Context0-ContextN
 - clarify javadoc of `get`
 - remove unneeded Nullable imports / annotations

test the Context.of methods + javadoc

fix isEmpty detecting Context0 instances (internal only) + test Context0

clarify javadoc of get (throws rather than returning null)

remove unneeded Nullable annotations/imports
@simonbasle
Copy link
Member

@smaldini following @rwinch comment on getOrDefault I reinstated the nullabity of the default. Kept it as a separate commit for easy drop / revert, and rebased the rest, grouping your changes, my changes and an unrelated one (so this whole PR is ready for a rebase-and-merge).

@simonbasle simonbasle added warn/api-change Breaking change with compilation errors and removed wip labels Aug 24, 2017
@smaldini smaldini merged commit 5d5872e into master Aug 28, 2017
smaldini pushed a commit that referenced this pull request Aug 28, 2017
 - rename contextStart to subscriberContext
 - remove contextGet in favor of existing handle
 - make most of Context API non nullable
 - add hasKey : boolean
 - add getOrEmpty : Optional<T>
smaldini pushed a commit that referenced this pull request Aug 28, 2017
 - add Context.of static factory methods for cardinalities 1-5
 - add specific implementations for cardinalities 1-5
 - add Context#putAll(Context)
 - fix isEmpty to also detect (internal only) instances of Context0
 - add tests for Context0-ContextN
 - clarify javadoc of `get`
 - remove unneeded Nullable imports / annotations

test the Context.of methods + javadoc

fix isEmpty detecting Context0 instances (internal only) + test Context0

clarify javadoc of get (throws rather than returning null)

remove unneeded Nullable annotations/imports
@smaldini smaldini deleted the context-api-improvements branch August 28, 2017 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement warn/api-change Breaking change with compilation errors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants