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 getOptional to Http context #16

Merged
merged 2 commits into from
Oct 27, 2017
Merged

Conversation

taer
Copy link
Contributor

@taer taer commented Oct 23, 2017

No description provided.

Copy link
Contributor

@mikkokar mikkokar left a comment

Choose a reason for hiding this comment

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

I would deprecate the old get() method. What do you think @kvosper @alobodzki ?

@@ -57,6 +59,11 @@ public void add(String key, Object value) {
return null;
}
};

default <T> Optional<T> getOptional(String key, Class<T> clazz){
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use a name like getIfAvailable rather than getOptional, as it is better to describe the behaviour of the method rather than simply the return type, which is already visible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine for me. I'll update w/ the change(and deprecation)

@kvosper
Copy link
Contributor

kvosper commented Oct 27, 2017

@mikkokar I agree with the deprecation. I have also added a comment to the file regarding the naming of the proposed new method.

@mikkokar mikkokar merged commit a38a98d into ExpediaGroup:master Oct 27, 2017
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.

3 participants