yearn / yearn-sdk

🦧 SDK for the yearn.finance platform. WIP
https://npm.im/@yfi/sdk
MIT License
53 stars 56 forks source link

fix: change if statement to switch for fetching token balances #234

Closed MahmoudHamdy02 closed 2 years ago

MahmoudHamdy02 commented 2 years ago

Changed if statement to switch for issue #230.

MahmoudHamdy02 commented 2 years ago

I have updated the error message, please let me know if it was committed correctly.

karelianpie commented 2 years ago

I have updated the error message, please let me know if it was committed correctly.

You've done more than changing the throw to a console.error, the changes you've introduced now belong in the other PR. Could you please remove them from this PR and here just have the convert if to a switch statement and instead of throwing having it console.error?

MahmoudHamdy02 commented 2 years ago

I had a bit of trouble with the previous commits but it should be good now, thanks and sorry for the trouble!

karelianpie commented 2 years ago

I had a bit of trouble with the previous commits but it should be good now, thanks and sorry for the trouble!

Please see my suggestion here https://github.com/yearn/yearn-sdk/pull/234/files#r819345715

I think you'd have to update the test as well, if you don't want to update the test maybe it's better to continue throwing and then with my PR we can change it to console.error

MahmoudHamdy02 commented 2 years ago

I have reverted it back to throwing the error, please let me know if the code is alright!

MahmoudHamdy02 commented 2 years ago

It's my first time using yarn, I installed it and ran yarn lint and yarn lint --fix, should hopefully be good now.

jstashh commented 2 years ago

@MahmoudHamdy02 there are some tests failing, I think we need to change the syntax to

case 1:
case 1337: {
  // ...
}
MahmoudHamdy02 commented 2 years ago

I've changed the syntax, please let me know if this works.

jstashh commented 2 years ago

thanks @MahmoudHamdy02 🙂