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

horizonclient: update fund method #1211

Closed
wants to merge 1 commit into from

Conversation

poliha
Copy link
Contributor

@poliha poliha commented Apr 30, 2019

This PR updates the Fund method that is used to fund new accounts on testnet.
It now uses the internal http client which allows us to set the app headers.
Also it returns TransactionSuccess struct instead of a http response.

Closes #1201

@poliha poliha added the horizonclient tag for new horizon client located in clients/horizonclient label Apr 30, 2019
Copy link
Contributor

@bartekn bartekn left a comment

Choose a reason for hiding this comment

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

It looks good however there is one thing I'd like to discuss. We need to figure out which versioning system we want to use. If we want to use semver we should follow it's rules. According to semver two changes of the API in this PR shouldn't be part of 1.0.1:

The other option is to actually rename 1.0.0 release to 0.1.0 which is considered an unstable API (https://semver.org/#spec-item-4) and stay at 0.x.x until we're confident that feature set for 1.0.0 is complete and there will be no API breaking changes. However, it's not a good practice.

@poliha
Copy link
Contributor Author

poliha commented Apr 30, 2019

I think we should use semver, and with that your observations are correct.
As tempting as renaming the release to 0.1.0 is, I think we shouldn't do that because as you said it is not good practice and we already have 1.0 out. I would prefer if we continue to release versions in accordance with semver rules for the changes that we make.

Looping in @tomquisel and @ire-and-curses for their thoughts on this

@poliha poliha force-pushed the horizonclient-fund branch from 4234c26 to e7c033f Compare April 30, 2019 14:23
@poliha poliha changed the base branch from master to horizonclient-v2.0.0 April 30, 2019 14:25
@poliha poliha closed this Apr 30, 2019
@poliha
Copy link
Contributor Author

poliha commented Apr 30, 2019

moved to #1213

@ire-and-curses
Copy link
Member

I agree with the points. Given that this is a minor helper method and no one depends on this library yet, can we bend the rule and just increment minor version this time? It seems strange to me to announce to the world a version which is already at 2+.

@bartekn
Copy link
Contributor

bartekn commented Apr 30, 2019

It seems strange to me to announce to the world a version which is already at 2+.

We won't announce it yet, but just keep adding features. When there is "enough" to release V2 we can do it.

Given that this is a minor helper method and no one depends on this library yet, can we bend the rule and just increment minor version this time?

I guess we can do this as an exception one time but it would be great to follow the semver from now on. I think that once people will start using it we may want to do some other breaking changes that work better for devs (just guessing, don't have any specific function in mind) so we'll have to start working on that 2.0.0 branch anyway.

@ire-and-curses
Copy link
Member

Yeah the reason I say this is that I am finishing up the public blog post today, and we have several minor version changes (e.g. #1198) that we may as well fold in if they're ready before announcing. So I am thinking of incrementing to 1.1 before announcing. I think it will be annoying for users if we change the fund() API very soon after release in a 2.0 release. So I think this next release is a special case. Definitely will do this correctly from now on though.

@bartekn
Copy link
Contributor

bartekn commented Apr 30, 2019

Sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
horizonclient tag for new horizon client located in clients/horizonclient
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants