-
Notifications
You must be signed in to change notification settings - Fork 303
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
coinmarketcap typescript refactor #218
Conversation
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
@emmick4 any updates on this. We should get this in this week, as we are looking to finish the TS migration and publish a new release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good, left only one potential change.
The price endpoint though is quite complicated. We might need to add various test cases regarding slug/cid + to do some good acceptance testing. There is probably room for refactoring, either now or at some point, once we cover all cases with tests.
@@ -24,6 +24,14 @@ export const toObjectWithNumbers = (obj: any) => { | |||
return Object.fromEntries(Object.entries(obj).map(([k, v]) => [k, toNumber(v)])) | |||
} | |||
|
|||
// pick a random string from env var after splitting with the delimiter ("a&b&c" "&" -> choice(["a","b","c"])) | |||
export const getRandomEnv = (name: string, delimiter = ',', prefix = '') => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment in the batch PR about this -> #226 (comment)
What do you think?
Agreed that there is probably room for refactoring and additional tests, but for the sake of staying in the scope of the TS refactor and hitting this release I think we should instead open a new ticket for that. Opening one now for CMC price endpoint refactor. |
Entering changes for acceptance testing
Refactor the Coinmarketcap external adapter to match the typescript adapter pattern.