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

Add nullability annotation for kotlin interop. #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add nullability annotation for kotlin interop. #45

wants to merge 2 commits into from

Conversation

jaychang0917
Copy link

Currently the oldState will be interpreted as nullable in Kotlin like:

class TestAction: Action<String> {
  override fun newState(oldState: String?): String {
     // ...
  }
}

This PR adds nullability annotation for better nullability interpretion in Kotlin.

@codecov-io
Copy link

codecov-io commented Dec 15, 2018

Codecov Report

Merging #45 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #45   +/-   ##
=======================================
  Coverage   94.59%   94.59%           
=======================================
  Files           5        5           
  Lines          74       74           
  Branches        5        5           
=======================================
  Hits           70       70           
  Misses          3        3           
  Partials        1        1
Impacted Files Coverage Δ
...rox-core/src/main/java/com/groupon/grox/Store.java 100% <100%> (ø) ⬆️

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 330a0ad...72d6c94. Read the comment docs.

@chenthorne
Copy link
Contributor

Is this actually true though? In Java land, why couldn't you have a null state?

If we want to enforce a non-null state need to update annotations for the appropriate Store classes too.

@jaychang0917
Copy link
Author

jaychang0917 commented Dec 18, 2018 via email

@chenthorne
Copy link
Contributor

Don't get me wrong, I'm for the change as I have the same problem in kotlin too but I could understand that a null-state could be a thing.

Grox isn't UI specific just helpful for it, it's just state management.

@chenthorne
Copy link
Contributor

If you're up for more annotations, I'd support adding them to the Rx classes as well :)

@stephanenicolas
Copy link
Collaborator

stephanenicolas commented Dec 19, 2018 via email

@alin-turcu
Copy link
Collaborator

Looks good on my side also. I don't think a null state would make much sense. I would rather have a "empty state" than a null one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants