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 flow type errors #811

Merged
merged 2 commits into from
Nov 22, 2017
Merged

fix flow type errors #811

merged 2 commits into from
Nov 22, 2017

Conversation

FredKSchott
Copy link
Contributor

This gets flow passing again on the codebase (a result of running #810)

@@ -27,6 +27,7 @@ function toRippledAmount(amount: Amount): RippledAmount {
if (amount.currency === 'XRP') {
return xrpToDrops(amount.value)
}
// $FlowFixMe: amount.issuer is not a Amount type property. Safe to remove?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch. issuer is actually the term that we use in rippled. We should add issuer to the Amount type, and keep using it here. (No hurry, but in a future release, we may deprecate counterparty.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(There are other areas in the code that will need to be updated to accommodate this)

Copy link
Contributor Author

@FredKSchott FredKSchott Nov 17, 2017

Choose a reason for hiding this comment

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

@intelliot Say more about the other areas that need to be changed? They'll need to accommodate by setting issuer? Read from issuer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@FredKSchott Right. For example, here are places where I think we'll need to set issuer or read from issuer. We can do this in a later PR though.

Copy link
Collaborator

@intelliot intelliot left a comment

Choose a reason for hiding this comment

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

This is very nice! Just a couple minor changes needed.

import type {Settings} from './settings-types.js'

type Settings = {
passwordSpent?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we prefer to use commas instead of semicolons here.

Copy link
Collaborator

@intelliot intelliot left a comment

Choose a reason for hiding this comment

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

Aside from cleaning up counterparty/issuer, which we can do later, this looks great.

@@ -27,6 +27,7 @@ function toRippledAmount(amount: Amount): RippledAmount {
if (amount.currency === 'XRP') {
return xrpToDrops(amount.value)
}
// $FlowFixMe: amount.issuer is not a Amount type property. Safe to remove?
Copy link
Collaborator

Choose a reason for hiding this comment

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

@FredKSchott Right. For example, here are places where I think we'll need to set issuer or read from issuer. We can do this in a later PR though.

@intelliot intelliot requested a review from wilsonianb November 17, 2017 23:21
@intelliot
Copy link
Collaborator

We can clean up counterparty/issuer in a later PR :) Merging.

@intelliot intelliot merged commit f90617e into XRPLF:develop Nov 22, 2017
@FredKSchott
Copy link
Contributor Author

👍 👍 👍

@FredKSchott FredKSchott deleted the fix-flow-types branch November 22, 2017 01:34
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