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

Revise Documentation for the Migration Module. #55

Merged
merged 6 commits into from
Apr 27, 2020

Conversation

KtorZ
Copy link
Contributor

@KtorZ KtorZ commented Apr 22, 2020

Related Issue

#18

Summary

This PR:

  • Rewrites the documentation for the Migration module to be more general, removing references to Cardano-specific blockchain values.
  • Introduces type BatchSize, which makes the batch size argument easier to refer to in documentation, and provides a way for users to relate the idealBatchSize and selectCoins functions to one another.
  • Exposes idealBatchSize in the public API.

In addition, this PR:

  • Adds the @since 1.0.0 metadata attribute to each symbol exposed in the public API. (The necessity of this was discussed in an earlier review, and a consensus was reached that we will include an attribute for each symbol in the public API, right from the first release.)

@KtorZ KtorZ self-assigned this Apr 22, 2020
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Does putting "since 1.0.0" on every single symbol add anything useful?

It would be good to have "since 1.1.0" on new functions in the next version though.

src/library/Cardano/CoinSelection/Migration.hs Outdated Show resolved Hide resolved
src/library/Cardano/CoinSelection/Migration.hs Outdated Show resolved Hide resolved
src/library/Cardano/CoinSelection/Migration.hs Outdated Show resolved Hide resolved
src/library/Cardano/CoinSelection/Migration.hs Outdated Show resolved Hide resolved
@jonathanknowles
Copy link
Contributor

@KtorZ
Copy link
Contributor Author

KtorZ commented Apr 23, 2020

@rvl At this stage it does not. But it'll be only useful by the time we make a second release indeed.

KtorZ and others added 5 commits April 27, 2020 08:54
This makes the batch size argument easier to refer to in documentation
for the `Migration.selectCoins` function, and provides a way to relate
`idealBatchSize` to `selectCoins`.
Remove Cardano-specific blockchain constants, and write a more general
description that could be applied to any UTxO-based blockchain.
@jonathanknowles
Copy link
Contributor

@rvl wrote:

Does putting "since 1.0.0" on every single symbol add anything useful?

After an internal team discussion, it was decided to include this attribute on every symbol exported in the public API right from the first published version.

@jonathanknowles
Copy link
Contributor

@jonathanknowles wrote:

I suspect this part could also be revised, as the values seem very specific to Cardano:

https://github.com/input-output-hk/cardano-coin-selection/blob/f62d069062f196d3b251ac6e7a039a0030d929d6/src/library/Cardano/CoinSelection/Migration.hs#L19-L21

Fixed in 0bab2e8.

@jonathanknowles jonathanknowles changed the title Small doc tweaks Revise Documentation for the Migration Module. Apr 27, 2020
Copy link
Contributor Author

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

👍

-- wallet to another.
--
-- Since UTxO-based blockchains typically impose limits on the sizes of
-- individual transactions, and since individual UTxO sets can contain
Copy link
Contributor

Choose a reason for hiding this comment

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

UTxO sets ot UTxO set ?

Copy link
Contributor

Choose a reason for hiding this comment

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

UTxO sets ot UTxO set ?

This is correct, because we've written:

"individual UTxO sets can contain arbitrarily many entries."

(The sentence is referring to the concept of UTxO sets in general.)

We could also have written:

"an individual UTxO set can contain arbitrarily many entries."

(Either would be correct.)

Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

lgtm!

@jonathanknowles jonathanknowles merged commit a87afe4 into master Apr 27, 2020
@jonathanknowles jonathanknowles deleted the KtorZ/small-doc-tweaks branch April 27, 2020 10:27
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.

None yet

4 participants