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

Fix handling a port number in URI #29

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

ksugar
Copy link
Contributor

@ksugar ksugar commented Apr 17, 2024

Properly handle a URI with port number.
The constructor that takes the authority needs one more argument.

// With previous implementation, this constructor is called
    public URI(String scheme, String host, String path, String fragment)
        throws URISyntaxException
    {
        this(scheme, null, host, -1, path, null, fragment);
    }

// With this PR, this constructor is called
    public URI(String scheme,
               String authority,
               String path, String query, String fragment)
        throws URISyntaxException
    {
        String s = toString(scheme, null,
                            authority, null, null, -1,
                            path, query, fragment);
        checkPath(s, scheme, path);
        new Parser(s).parse(false);
    }

Properly handle a URI with port number.
The constructor that takes the authority needs one more argument.
@cmhulbert
Copy link
Contributor

Thanks for this, that is a frustrating constructor issue, nice catch.
Do you have a small test that could verify the behavior? I'm happy to accept this now, since I think this is a true bug, but it would be nice to have a test to avoid a regression in the future.

@cmhulbert cmhulbert merged commit 7cd782d into saalfeldlab:master Apr 17, 2024
3 checks passed
@ksugar ksugar mentioned this pull request Apr 18, 2024
@ksugar
Copy link
Contributor Author

ksugar commented Apr 18, 2024

@cmhulbert thanks for the review. I sent another PR for the test.
#30

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.

2 participants