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

Fix issues with new nullability annotations #318

Closed
4 tasks done
ljacqu opened this issue Jun 20, 2023 · 3 comments
Closed
4 tasks done

Fix issues with new nullability annotations #318

ljacqu opened this issue Jun 20, 2023 · 3 comments
Milestone

Comments

@ljacqu
Copy link
Member

ljacqu commented Jun 20, 2023

This issue handles short-term issues that block a release; long-term issues will be handled in #319.

Issues:

  • Fix line length issues (check with CodeClimate)
  • Ensure that the build works locally again -> IntelliJ picks up its notnull annotation and adds bytecode that rejects null arguments
  • Property values are sometimes declared as notnull, sometimes as nullable (e.g. ConfigurationData#setValue is not-null, vs. #getValue is nullable... huh?). See also export values, BeanPropertyType allows null in toExportValue but it is not allowed in ListProperty
    • Not sure what the ideal thing is here, because BaseProperty impl. should never be null, and pretty much for all the use cases I envision for ConfigMe, property values aren't null. Maybe go all in and just declare them not-null everywhere? After all, Property<Optional<T>> is promoted for any cases where we want to skip having values.
  • PropertyBuilder#type is null for the inline array implementation. Adapt.
@ljacqu ljacqu added this to the 1.4.0 Release milestone Jun 20, 2023
ljacqu added a commit that referenced this issue Jun 20, 2023
ljacqu added a commit that referenced this issue Jun 20, 2023
… be null even if we don't expect them to be
ljacqu added a commit that referenced this issue Jun 20, 2023
- ConfigMe doesn't provide null values for properties by default, but is somewhat "agnostic" at allowing other property implementations doing so. This is difficult to represent with the annotations, so some have been replaced by a comment "PV" (for property value) that should always be not null by default, but we cannot technically guarantee it.
@ljacqu
Copy link
Member Author

ljacqu commented Jun 20, 2023

It's a core concept of ConfigMe to not return null as property values since we'll fall back on the default value if we don't have any other knowledge. Nevertheless, I'm a little hesitant to flat out deny the possibility of having null as values if someone wants to use ConfigMe in such a way—as such, I've removed some @NotNull annotations from property values, which is not great because that's exactly what you'll get into contact with during normal integrations of ConfigMe.
I'm thinking maybe we could overload the methods cleverly to have @NotNull on another method named the same, and maybe have a marker interface?? It's a little overkill but I don't know how to keep the best of both worlds. I like the marker interface better than a concrete SettingsManager implementation that checks that all values are not null (or that all properties extend BaseProperty) because the marker interface can be used for other property implementations.

@ljacqu ljacqu closed this as completed Jun 20, 2023
@ljacqu ljacqu reopened this Jun 20, 2023
@ljacqu
Copy link
Member Author

ljacqu commented Jun 20, 2023

Need to sleep on this. In the spirit of keeping it simple, maybe we should go all in and disallow null values. I think it's one of the core strengths of ConfigMe: being able to get values without needing to worry about nullness. Otherwise, Optional exists and is supported, as would be other home-brewed value "wrappers"

ljacqu added a commit that referenced this issue Jun 21, 2023
- Undo most of 4c35833: consider property values to always be not-null as to benefit from type safety. Correct usage of ConfigMe should never return null as property value; instead, a suitable wrapper like Optional should be used.
@ljacqu
Copy link
Member Author

ljacqu commented Jun 21, 2023

Changed to consider property values not null. I think it works out best like this. Only odd remnant is this method in Property:

boolean isValidValue(@Nullable T value);

^ The base impl. makes sure that the value is not null. If I wrote this code today, I would do the null check (if at all) outside, e.g. in the SettingsManager itself. Might be changed in ConfigMe 2.0 to do that. For now I leave it because BaseProperty extensions might call #isValidValue internally (I know I've done so at least once in other projects)

@ljacqu ljacqu closed this as completed Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant