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

Make 'none+' serialization format as optional. #2241

Merged
merged 17 commits into from Nov 12, 2019
Merged

Conversation

rmohta
Copy link
Contributor

@rmohta rmohta commented Nov 8, 2019

Motivation:

Improve API UX, by not forcing application to provide none+ serialization format.

Modifications:

  • Update Scheme.parse() and tryParse() to default to none+, if no serialization format is passed.
  • Update DefaultClientFactory to normalize URI by removing none+ from URI scheme.

Result:

Copy link
Collaborator

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks!

@codecov
Copy link

codecov bot commented Nov 10, 2019

Codecov Report

Merging #2241 into master will increase coverage by 0.03%.
The diff coverage is 84.21%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2241      +/-   ##
============================================
+ Coverage      73.5%   73.54%   +0.03%     
- Complexity     9747     9766      +19     
============================================
  Files           849      852       +3     
  Lines         37575    37628      +53     
  Branches       4616     4631      +15     
============================================
+ Hits          27620    27674      +54     
+ Misses         7587     7578       -9     
- Partials       2368     2376       +8
Impacted Files Coverage Δ Complexity Δ
.../linecorp/armeria/client/DefaultClientFactory.java 76.66% <100%> (+3.08%) 19 <3> (+3) ⬆️
.../main/java/com/linecorp/armeria/common/Scheme.java 86.48% <100%> (+12.2%) 16 <2> (+3) ⬆️
...com/linecorp/armeria/client/HttpClientBuilder.java 71.05% <100%> (ø) 9 <1> (ø) ⬇️
...linecorp/armeria/client/AbstractClientFactory.java 41.66% <25%> (ø) 6 <0> (ø) ⬇️
...n/java/com/linecorp/armeria/client/UserClient.java 72.72% <0%> (-11.28%) 11% <0%> (+2%)
.../linecorp/armeria/server/ServiceConfigBuilder.java 57.44% <0%> (-10.89%) 15% <0%> (-7%)
...ava/com/linecorp/armeria/server/ServiceConfig.java 62.71% <0%> (-3.33%) 21% <0%> (+2%)
...om/linecorp/armeria/server/VirtualHostBuilder.java 56.81% <0%> (-1.85%) 59% <0%> (-6%)
...linecorp/armeria/server/ServiceBindingBuilder.java 64.86% <0%> (-1.81%) 20% <0%> (ø)
.../java/com/linecorp/armeria/server/VirtualHost.java 54.4% <0%> (-1.61%) 31% <0%> (-1%)
... and 46 more

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 a582d86...1e29419. Read the comment docs.

@rmohta
Copy link
Contributor Author

rmohta commented Nov 11, 2019

nit: Remove public here and the following methods?

Done. Removed in ClientBuilderTest too.

@ikhoon
Copy link
Contributor

ikhoon commented Nov 11, 2019

Would you be open, if I open another PR later to add this as a checkstyle check (to prevent this in future)?

We had chat and decided to need more discussion on this issue. I filed an issue for that. #2247
Thank you for nice suggestion and if you have a good idea, leave your opinion on it :-)

@ikhoon
Copy link
Contributor

ikhoon commented Nov 11, 2019

I have made changes to handle UNDEFINED_URI, but haven't reverted the value of constant. It's because, per this PR - we don't want uri() to return none+.
I think, it would not be idiomatic, to have none+ returned for this special case. We are doing an == check for this constant though.
WDYT?

That is a nice idea! Removing none+ from UNDEFINED_URI is good for me too. Thanks for the detail :-)

Copy link
Collaborator

@trustin trustin left a comment

Choose a reason for hiding this comment

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

LGTM once others' comments are addressed.

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Great work + thanks for the fast feedback! 👍 @rmohta

@trustin trustin merged commit e21448a into line:master Nov 12, 2019
@trustin
Copy link
Collaborator

trustin commented Nov 12, 2019

🎉 Thanks a lot, @rmohta!

@rmohta rmohta deleted the issue-2219 branch November 13, 2019 15:10
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
Motivation:

Improve API UX, by not forcing application to provide none+ serialization format.

Modifications:

- Update `Scheme.parse()` and `tryParse()` to default to `none+`, if no serialization format is passed.
- Update `DefaultClientFactory` to normalize URI by removing `none+` from URI scheme.

Result:

- Fixes line#2219
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept URIs without none+ and return URIs without none+
5 participants