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: add more documentation #1226

Merged
merged 4 commits into from
May 2, 2019

Conversation

poliha
Copy link
Contributor

@poliha poliha commented May 2, 2019

This PR adds more comments indicating required and optional fields in the exported structs.
Closes #1206

@poliha poliha added the horizonclient tag for new horizon client located in clients/horizonclient label May 2, 2019
@poliha poliha requested a review from ire-and-curses May 2, 2019 09:54
Copy link
Member

@ire-and-curses ire-and-curses left a comment

Choose a reason for hiding this comment

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

Looks good! Minor comments.

HTTP HTTP
horizonTimeOut time.Duration
AppName string
// URL of Horizon server to connect
Copy link
Member

Choose a reason for hiding this comment

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

Does this look ok in the godoc? Otherwise we could move these comments to the right hand side of the field names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea it looks good. Tested with godocs locally

type AccountRequest struct {
AccountID string
DataKey string
}

// EffectRequest struct contains data for getting effects from a horizon server.
// ForAccount, ForLedger, ForOperation and ForTransaction: Not more than one of these can be set at a time. If none are set, the default is to return all effects.
// The query parameters (Order, Cursor and Limit) can all be set at the same time
// "ForAccount", "ForLedger", "ForOperation" and "ForTransaction": Not more than one of these can be set at a time. If none are set, the default is to return all effects.
Copy link
Member

Choose a reason for hiding this comment

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

Can we break up this long line?

@@ -263,13 +263,6 @@ func (c *Client) Assets(request AssetRequest) (assets hProtocol.AssetsPage, err
return
}

// Stream is for endpoints that support streaming
Copy link
Member

Choose a reason for hiding this comment

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

Why do we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer used. I must have missed removing this earlier

@poliha poliha merged commit 1e78abf into release-horizonclient-v1.1.0 May 2, 2019
@poliha poliha deleted the horizonclient-moredocs branch May 2, 2019 16:59
poliha added a commit that referenced this pull request May 3, 2019
* Remove old stream method

* more comments

* fix format

* split comment
poliha added a commit that referenced this pull request May 3, 2019
* horizonclient/txnbuild README fixes (#1210)

* fix client links in top-level readme

* update clients README

* mark old client deprecated in godoc short summary

* fix code of conduct, standardise example

* fix code of conduct/contributing links

* txnbuild: enables multiple signatures (#1198)

This PR enables multiple signatures on a transaction in the new Go SDK. It also lets every `Operation` type have a different source account than its `Transaction`. These changes are intertwined. Without multiple signatures, every operation in a transaction must share the transaction's source account. Differing source accounts are the most common use case for multiple signatures, and they also test it with greatest completeness.

* root repo changelog links to sub-projects (#1214)

* keystore: add update-keys endpoints to spec (#1215)

We need an endpoint to update the encrypted seed when users forget their
passwords.

* move keystore to exp to fix build (#1223)

* Add minimal files to fix build (#1225)

* exp/ticker: Orderbook data support (#1193)

* exp/ticker: update partial market query to also output base/counter asset types

* exp/ticker: add functions to scrape orderbook data

* exp/ticker: update orderbookstats param names to match market standards

* exp/ticker: create orderbookstats database model and migrations

* exp/ticker: consolidate upsert logic and implement orderbookstats upsert

* exp/ticker: create CLI command to ingest orderbooks

* exp/ticker: update horizon SDK path

* exp/ticker: add orderbook stats to market.json generator

* exp/ticker: add separate query to retrieve relevant markets

* exp/ticker: add orderbook stats to graphql interface

* exp/ticker: add Docker support for orderbook ingestion

* exp/ticker: fix partial aggregated market query + add tests

* exp/ticker: fix globall aggregated market query + add tests

* exp/ticker: update ticker binary link and fix crontab comment

* exp/ticker: fix issue that would enable inf values to be stored in db

* exp/ticker: ensure only store valid orderbook entries are stored

* exp/ticker: format gql/static/bindata.go to prevent CI errors

* exp/ticker: add updated bindata with pg 9.5-compatible migrations

* exp/ticker: ensure markets with 0 orderbook entries can be handled by tickerdb

* exp/ticker: remove unnecessary commas from graphql schema

* exp/ticker: consolidate bid and ask positions on field names

* exp/ticker: simplify logic for creating orderbook requests

* exp/ticker: fix how bid and ask volumes are calculated

* exp/ticker: allow negative spread values

* exp/ticker: fix close_time fallback on markets query

* exp/ticker: add page size limit to orderbook requests

* Make PR template a bit more helpful. (#1238)

* Make PR template a bit more helpful.

Clean up some typos, and point to an example docs folder in the go repo
rather than just the external docs hosted on our site.

* Fix typo.

* Agh, formatting.

* horizonclient: update Fund method (#1213)

* update client.Fund

* update change log

* fix space

* horizonclient: add more documentation (#1226)

* Remove old stream method

* more comments

* fix format

* split comment

* horizonclient: Set default HTTP client (#1228)

* set default HTTP client

* changes from review

* changelog for txnbuild 1.1 (#1231)

* horizonclient/txnbuild README fixes (#1210)

* fix client links in top-level readme

* update clients README

* mark old client deprecated in godoc short summary

* fix code of conduct, standardise example

* fix code of conduct/contributing links

* txnbuild: enables multiple signatures (#1198)

This PR enables multiple signatures on a transaction in the new Go SDK. It also lets every `Operation` type have a different source account than its `Transaction`. These changes are intertwined. Without multiple signatures, every operation in a transaction must share the transaction's source account. Differing source accounts are the most common use case for multiple signatures, and they also test it with greatest completeness.

* root repo changelog links to sub-projects (#1214)

* keystore: add update-keys endpoints to spec (#1215)

We need an endpoint to update the encrypted seed when users forget their
passwords.

* move keystore to exp to fix build (#1223)

* Add minimal files to fix build (#1225)

* changelog for txnbuild 1.1

* update change log (#1233)

* horizonclient: add version (#1229)

* add package version

* fix go fmt

* clients/horizonclient: support dynamic effects (#1217)

* update effects in protocols/horizon

* update client interface

* update tests

* update changelogs

* fix typo
bartekn pushed a commit that referenced this pull request May 6, 2019
* Remove old stream method

* more comments

* fix format

* split comment
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.

2 participants