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

support ftp-urls in WebClient #625

Merged
merged 7 commits into from
May 18, 2017

Conversation

alexlehm
Copy link
Contributor

add same url handling to WebClient for ftp-urls as was added to vertx-core in the pr eclipse-vertx/vert.x#1959

the code probably needs some improvements, e.g. constructing the url with a URL constructor instead of creating the String directly

(not quite finished, the solution is somewhat complicated since the url is parsed twice)
@alexlehm alexlehm changed the title Web client protocol support ftp-urls in WebClient May 11, 2017
Signed-off-by: alexlehm <alexlehm@gmail.com>
Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

LGTM. Can you remove the empty Javadoc block in the test?

URI uri = new URI(protocol, null, host, port, requestURI, null, null);
req = client.requestAbs(method, uri.toString());
} catch (URISyntaxException ex) {
// FIXME: what is proper error handling?
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like the right way to handle the error to me.

import io.vertx.ext.unit.junit.VertxUnitRunner;

/**
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove ?

remove empty comment from unit test class

Signed-off-by: alexlehm <alexlehm@gmail.com>
@alexlehm
Copy link
Contributor Author

@tsegismont actually test unit test isn't really useful as it relies on Squid running on the same machine, if we want to test it, we need a fake proxy as in core http tests

@tsegismont
Copy link
Contributor

tsegismont commented May 16, 2017 via email

Signed-off-by: alexlehm <alexlehm@gmail.com>
@alexlehm
Copy link
Contributor Author

alexlehm commented May 17, 2017

turns out that is already available since WebClientTest extends HttpTestBase from vertx-core
I have changed the test to use the test proxy and added it to the test class

Signed-off-by: alexlehm <alexlehm@gmail.com>
@tsegismont
Copy link
Contributor

LGTM. Thanks @alexlehm

@tsegismont tsegismont merged commit 5bc18fa into vert-x3:master May 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants