CT-403: Add type information to xlm#390
Conversation
Codecov Report
@@ Coverage Diff @@
## master #390 +/- ##
===========================================
+ Coverage 38.47% 60.82% +22.35%
===========================================
Files 73 74 +1
Lines 9805 9837 +32
Branches 2138 2144 +6
===========================================
+ Hits 3772 5983 +2211
+ Misses 5392 2917 -2475
- Partials 641 937 +296
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
i'm not sure either
I have a feeling we don't need it, but maybe I'm missing something? @CapnMigraine any ideas? Probably a good idea to have you review this change anyway.
There was a problem hiding this comment.
No reason, we don't need it. Please remove it.
d00301f to
c95bf71
Compare
CapnMigraine
left a comment
There was a problem hiding this comment.
Thanks for doing this, it's looking much better!
Are we encouraging the use of trailing commas in the SDK? I ask because there are a few missing here, maybe it's a low-hanging fruit for this PR.
There was a problem hiding this comment.
what is this type? I think it should be string
There was a problem hiding this comment.
It's a type exported by the stellar sdk.
Here's the definition:
enum MemoType
{
MEMO_NONE = 0,
MEMO_TEXT = 1,
MEMO_ID = 2,
MEMO_HASH = 3,
MEMO_RETURN = 4
};
Do you think it makes sense to expose this stellar SDK type as a type in bitgojs? Or should we redefine or rename it? Or just make the type string and leave it?
There was a problem hiding this comment.
Well, in the end they're all strings. I think we can simplify it for readability.
https://stellar.github.io/js-stellar-sdk/Memo.html
There was a problem hiding this comment.
No reason, we don't need it. Please remove it.
There was a problem hiding this comment.
this list doesn't have a type
There was a problem hiding this comment.
There won't ever be any elements of this array, so the type is actually a correct empty array literal. Here's more info if you are curious, since I haven't seen this sort of feature in other languages: https://basarat.gitbooks.io/typescript/docs/types/literal-types.html
There was a problem hiding this comment.
ok, thanks for clarifying
Yes, I prefer trailing commas where allowed in es5, but this isn't enforced currently unless prettier is configured for the file in question. I'll add them to this PR as I come across them |
c95bf71 to
26978c7
Compare
CapnMigraine
left a comment
There was a problem hiding this comment.
LGTM.
The MemoType is a non-issue. I'll re-approve if you decide to change it.
There was a problem hiding this comment.
Well, in the end they're all strings. I think we can simplify it for readability.
https://stellar.github.io/js-stellar-sdk/Memo.html
There was a problem hiding this comment.
ok, thanks for clarifying
No description provided.