Skip to content

Remove Nullable annotation #15876

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

Closed
wants to merge 2 commits into from
Closed

Remove Nullable annotation #15876

wants to merge 2 commits into from

Conversation

wonwoo
Copy link
Contributor

@wonwoo wonwoo commented Feb 8, 2019

No description provided.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 8, 2019
@wilkinsona
Copy link
Member

Thanks for the PR. The intent of the proposed changes isn't clear to me. Can you please describe what you are aiming to harmonise? Note that we have an issue open to add @Nullable across Spring Boot's API. It is something that we want to do, but we want to do it in one go across the entire API.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Feb 8, 2019
@wonwoo
Copy link
Contributor Author

wonwoo commented Feb 8, 2019

nullable annotation is declared another convert method. in harmony with them..

But if you change the entire api at once, this pr is closed.

@wilkinsona
Copy link
Member

Thanks for the clarification. The existing usages of @Nullable on a Converter are only in some test code. I'm also not sure why it's there and think I could probably be removed.

@snicoll, can you recall why we have @Nullable here:

@Nullable
@Override
public Object convert(@Nullable Object source, TypeDescriptor sourceType,
TypeDescriptor targetType) {
if (source == null) {
return null;
}
return new ExampleId(UUID.fromString((String) source));
}

@snicoll
Copy link
Member

snicoll commented Feb 8, 2019

It is probably a mistake, IntelliJ IDEA can add the annotation automatically when you override a method.

@wonwoo
Copy link
Contributor Author

wonwoo commented Feb 8, 2019

@Override
@Nullable
public Object convert(@Nullable Object source, TypeDescriptor sourceType,
TypeDescriptor targetType) {
List<Object> list = Arrays.asList(ObjectUtils.toObjectArray(source));
return this.delegate.convert(list, sourceType, targetType);
}

ArrayToDelimitedStringConverter, DelimitedStringToArrayConverter, DelimitedStringToCollectionConverter

Nullable Annotations are declared in the class

@wilkinsona
Copy link
Member

wilkinsona commented Feb 8, 2019

Well spotted. They shouldn't be there either (and are providing no benefit as it's a package-private class). Would you like to repurpose this PR to remove them? The only place where @Nullable should be used is to allow actuator endpoint operations to indicate that an input is optional.

@wonwoo wonwoo changed the title Harmonize Nullable annotation Remove Nullable annotation Feb 8, 2019
@wilkinsona wilkinsona added type: task A general task and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Feb 11, 2019
@wilkinsona wilkinsona self-assigned this Feb 11, 2019
@wilkinsona wilkinsona added this to the 2.1.3 milestone Feb 11, 2019
wilkinsona pushed a commit that referenced this pull request Feb 11, 2019
wilkinsona added a commit that referenced this pull request Feb 11, 2019
@wilkinsona
Copy link
Member

Thanks very much for making your first contribution to Spring Boot, @wonwoo. The proposed changes have been merged into 2.1.x and forwards into master.

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

Successfully merging this pull request may close these issues.

4 participants