Skip to content

Method signature mismatched with javadoc and implementation #2587

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.

Sign up for GitHub

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

Jump to bottom
Merged
metacosm merged 3 commits into from
Nov 20, 2024
Merged

Method signature mismatched with javadoc and implementation #2587

metacosm merged 3 commits into from
Nov 20, 2024

Conversation

@robobario
Copy link
Contributor

@robobario robobario commented Nov 18, 2024 •
edited
Loading

The javadoc of ManagedDependentResourceContext#put describe it as returning Optional. The implementation returns an Optional but casts it to T, so it is currently a compile error to write:

Optional<String> result = new ManagedDependentResourceContext().put("key", "value");

Using the method as declared results in a ClassCastException:

DefaultManagedDependentResourceContext context = new DefaultManagedDependentResourceContext();
context.put("key", "value");
String actual = context.put("key", "valueB");

fails with:

java.lang.ClassCastException: class java.util.Optional cannot be cast to class java.lang.String (java.util.Optional and java.lang.String are in module java.base of loader 'bootstrap')

The user can cast to get at the Optional like Optional<String> actual = (Optional<String>) (Object) context.put("key", "value");

So this PR updated the javadoc and implementation to match the method declaration and return the object-or-null rather than an Optional.

@openshift-ci openshift-ci bot requested review from adam-sandor and csviri November 18, 2024 00:16
@robobario robobario mentioned this pull request Nov 18, 2024
Merged
@metacosm
Copy link
Collaborator

metacosm commented Nov 18, 2024

This should target the next branch because this would be a breaking API change. Alternatively, we would need to deprecate the existing methods and provide new implementations but this cannot go as-is in main.

@robobario
Copy link
Contributor Author

robobario commented Nov 19, 2024

pushed up creating a new method and deprecating the old, a bit clunky since put is a nicer name than the new one.

@metacosm metacosm self-requested a review November 19, 2024 08:44
@metacosm
Copy link
Collaborator

metacosm commented Nov 19, 2024

Upon further review, looking at the history of the modifications, I believe that this API got broken at some point in the past and we should probably conform to the Map-like API and change the documentation accordingly instead of returning Optional. I have made the required changes but I cannot push to your branch, @robobario.

@metacosm metacosm mentioned this pull request Nov 19, 2024
Closed
@metacosm
Copy link
Collaborator

metacosm commented Nov 19, 2024

@robobario see #2595 for the changes that I think should be included as part of this work.

@robobario
Copy link
Contributor Author

robobario commented Nov 19, 2024

great, thanks @metacosm . Have updated and cleaned up the git history (and renamed a test method)

csviri
csviri approved these changes Nov 20, 2024
View reviewed changes
Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

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

LGTM

@metacosm metacosm merged commit 1f6cf2c into operator-framework:main Nov 20, 2024
20 checks passed
@robobario
Copy link
Contributor Author

robobario commented Nov 20, 2024

thank you!

@metacosm
Copy link
Collaborator

metacosm commented Nov 20, 2024

Thank you for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@metacosm metacosm metacosm approved these changes

@csviri csviri csviri approved these changes

@adam-sandor adam-sandor Awaiting requested review from adam-sandor

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@robobario @metacosm @csviri