Skip to content

Don't modify signature of 'Connection' constructor. #25

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.

Sign up for GitHub

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

Jump to bottom
Closed
labkey-tchad wants to merge 3 commits into from
Closed

Don't modify signature of 'Connection' constructor. #25

labkey-tchad wants to merge 3 commits into from

Conversation

@labkey-tchad
Copy link
Member

@labkey-tchad labkey-tchad commented Feb 11, 2022 •
edited by labkey-jeckels
Loading

Rationale

A previous update changed the signature of one of the 'Connection' constructors. This broke compatibility with some usages that were catching the URISyntaxException it claimed to throw.
This makes the constructors inconsistent but fixing those up is a job for another day.

Related Pull Requests

Changes

  • Make 'Connection' constructor match signature from v1.4.0

labkey-jeckels
labkey-jeckels reviewed Feb 11, 2022
View reviewed changes
Copy link
Contributor

@labkey-jeckels labkey-jeckels left a comment

Choose a reason for hiding this comment

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

Is there a reason to reintroduce this exception instead of updating the JDBC side? As long as a change needs to be made, why not make them consistent instead?

@labkey-tchad
Copy link
Member Author

labkey-tchad commented Feb 11, 2022

Is there a reason to reintroduce this exception instead of updating the JDBC side? As long as a change needs to be made, why not make them consistent instead?

Just trying to follow semver rules. Keep it backward compatible for 1.5.0 then do a wider fixup round for 2.0 at some point.
Maybe this library isn't used widely enough to conform that closely to semantic versioning rules.

@labkey-jeckels
Copy link
Contributor

labkey-jeckels commented Feb 11, 2022

Makes sense. What about calling it 2.0.0? I know it's not a grand refactor or anything, but semver doesn't care.

labkey-jeckels
labkey-jeckels approved these changes Feb 15, 2022
View reviewed changes
labkey-client-api/src/org/labkey/remoteapi/query/ImportDataCommand.java Outdated
}

public static void main(String[] args) throws IOException
public static void main(String[] args) throws IOException, URISyntaxException
Copy link
Contributor

@labkey-jeckels labkey-jeckels Feb 15, 2022

Choose a reason for hiding this comment

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

Is this edit needed now?

@labkey-tchad
Copy link
Member Author

labkey-tchad commented Apr 18, 2022

Obviated by #26

@labkey-tchad labkey-tchad deleted the fb_javaApiVersion branch April 18, 2022 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@labkey-jeckels labkey-jeckels labkey-jeckels approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@labkey-tchad @labkey-jeckels