Skip to content

Conversation

@mihhail-lapushkin
Copy link
Contributor

Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I've added a couple of comments. It would be nice if that extra constructor was exercised by a test. Can you please add one?

private final String rootUri;

private final UriTemplateHandler handler;
private final UriTemplateHandler delegateHandler;
Copy link
Member

Choose a reason for hiding this comment

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

This rename sounds unnecessary for the task at hand. Can you please revert?

* @param environment the environment used to determine the port
* @param scheme the scheme of the root uri
* @param delegateHandler the delegate handler
* @since 2.0.X
Copy link
Member

Choose a reason for hiding this comment

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

since tag uses actual version. That would be 2.0.3.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label May 18, 2018
@snicoll
Copy link
Member

snicoll commented May 18, 2018

@mihhail-lapushkin thanks for the quick feedback. I did edit my comment to mention the addition of a test. Thoughts?

@mihhail-lapushkin
Copy link
Contributor Author

Sure, I can add some.

@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue labels May 18, 2018
@snicoll snicoll added this to the 2.0.3 milestone May 18, 2018
@snicoll snicoll self-assigned this May 18, 2018
snicoll added a commit that referenced this pull request May 18, 2018
* pr/13208:
  Polish contribution
  Support custom UriTemplateHandler in LocalHostUriTemplateHandler
@snicoll snicoll closed this in 300f6bf May 18, 2018
@snicoll
Copy link
Member

snicoll commented May 18, 2018

@mihhail-lapushkin thanks for the PR, it is now merged in 2.0.x and master.

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

Labels

type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants