Replace popsicle with isomorphic-fetch #4

radiovisual · 2017-06-11T08:30:58Z

This PR keeps the application running as expected, but also fixes the problem where running the tests is failing (possibly) due to nock + popsicle.

It also increases code coverage from 83% to ~~92%~~ 100%. 🎉

coveralls · 2017-06-11T08:38:29Z

Changes Unknown when pulling f7f8082 on radiovisual:fetch into ** on brh55:master**.

… fetch

coveralls · 2017-06-11T08:55:59Z

Changes Unknown when pulling 3b3f567 on radiovisual:fetch into ** on brh55:master**.

coveralls · 2017-06-11T08:55:59Z

Changes Unknown when pulling 3b3f567 on radiovisual:fetch into ** on brh55:master**.

coveralls · 2017-06-11T09:07:24Z

Changes Unknown when pulling a649039 on radiovisual:fetch into ** on brh55:master**.

brh55

Nice fetch is a great choice. Take a look at my comments and tell me what you think. 👏

brh55 · 2017-06-11T16:09:03Z

cli.js

@@ -26,7 +26,7 @@ const cli = meow(`
      USD: 261.91
 `);

-const input = cli.input.map(item => item.replace(/,|\s+/g, '')).join(',');
+const input = cli.input.map(item => item.replace(/,\s+/g, '')).join(',');


Looks like the same is logic is happening in the API(LOC 7), you should be able to just turn the array to a string and pass it to the API to format.

yes, you are right. No need to do this in more than one place.

brh55 · 2017-06-11T16:09:45Z

package.json

@@ -27,9 +27,9 @@
 		"cryptocurrency"
 	],
 	"dependencies": {
+    "isomorphic-fetch": "^2.2.1",


Looks like it needs a tab or space

coveralls · 2017-06-11T18:41:52Z

Changes Unknown when pulling ff8060f on radiovisual:fetch into ** on brh55:master**.

coveralls · 2017-06-11T19:00:46Z

Changes Unknown when pulling 3988026 on radiovisual:fetch into ** on brh55:master**.

radiovisual · 2017-06-11T19:04:41Z

I think I made all the changes you requested. GitHub is saying that there are still changes pending, but I think I covered everything. Let me know if I missed something.

brh55

Looks good!

brh55 · 2017-06-11T22:11:15Z

I'm thinking we could strip out the API to a core module such as (crypto-price) and make several different CLI for different cryptos. Since I would like to do one for Lisk as well.

radiovisual · 2017-06-11T23:20:06Z

I'm thinking we could strip out the API to a core module such as (crypto-price) and make several different CLI for different cryptos. Since I would like to do one for Lisk as well.

I like this idea. I would like to be able to compare different crypto-currencies from the command line, but also have the freedom to write applications in node to consume this data as well. I am happy to help if you want me to extract the API.

brh55 · 2017-06-12T02:21:16Z

I'm thinking we could make an org? Something crypto related, and perhaps move this over to it? Thoughts?

radiovisual · 2017-06-12T07:08:46Z

I'm thinking we could make an org? Something crypto related, and perhaps move this over to it?

I would love that, and I would be happy to help building crypto-related modules. Please start the org and send me an invite!

brh55 · 2017-06-12T14:49:19Z

@radiovisual Have any good names to call the org? lol

radiovisual · 2017-06-12T14:59:03Z

@brh55

If not already taken:

crypto
crypt

Only if its cryptocoin-centric:

cryptocoin (only if its cryptocoin-centric)
cryptoin
ccoin

I also like:

cryto (read "cry-dough")
krypto
kriptoin

Maybe I will think of something better later on...

brh55 · 2017-06-12T15:06:26Z

Available Options:

crypto-devs
crydo
cryptdoe
dcryptos
crypto-code
krycode
crypto-tools
cctools

radiovisual · 2017-06-12T15:10:41Z

Dang, all the good names are taken. Looks like we will need to get creative. haha.

I like crypto-code, but cryptocode is already taken, and I think that could lead to some namespace confusion.

radiovisual · 2017-06-12T15:11:33Z

Maybe we can find some cool keywords from the decentralized and cryptographic keyword space and find something unique that hasn't been taken yet, but still feels relevant.

radiovisual · 2017-06-12T15:17:05Z

These are all available:

crypt-chart
crypt-space
crypti
cryp-tip
cryptools
crypline
cryp-line
crypterm
cryp-term
cryminal 🔫

brh55 · 2017-06-12T15:18:56Z

Yeah, we might need a interesting name completely different from "crypto". Also crypto-craft is an option

radiovisual · 2017-06-12T15:19:21Z

crypto-craft is cool.

brh55 · 2017-06-12T15:30:43Z

Haha I just like the shortness of crypti 💃

radiovisual · 2017-06-12T15:33:31Z

I really like crypti as well. 👍 😄 🌮

Makes us feel relevant and serious, but super hipster and cool at the same time. 🤣

radiovisual · 2017-06-12T15:35:25Z

I like crypto-craft and crypti so far the best.

brh55 · 2017-06-12T15:36:02Z

Invited you for crypti 🍡 👍 , transferring this repo over now

radiovisual · 2017-06-12T15:37:37Z

Alright! Now we can get to work. 🕺 🚀

brh55 · 2017-06-12T15:38:58Z

Made you owner as well, feel free to re-word the org description and add a logo. 🎢

radiovisual · 2017-06-12T15:39:32Z

Made you owner as well, feel free to re-word the org description and add a logo

Ok, awesome. Thanks!

radiovisual · 2017-06-12T15:53:31Z

So do you want a separate command line tool for each crypto currency, or one tool that can manage them all?

It would be cool if we had a tool that could compare cryptocurrencies like we pair currencies in Forex:

$ ccoin eth/usd, btc/usd
Converting...
   ETH/USD: 000.00
   BTC/USD: 0000.00

Or something like: $ ccoin <from> <to>

$ ccoin eth,btc usd,jpy
Converting...
   ETH/USD: 000.00
   ETH/JPY: 000.00
   BTC/USD: 0000.00
   BTC/JPY: 0000.00

Or even cooler, if you don't remember all the coin codes, you can have an interface that helps you decide:

$ ccoin
Which cryptocurrencies do you want?
  [x] Ethereum
  [ ] BitCoin

What currencies do you want to convert to?
 [x] American Dollar (USD)
 [ ] Japanese Yen (JPY)

ETH/USD: 000.00

brh55 · 2017-06-12T16:01:34Z

Whew the last option is awesome. I'm thinking we have one core CLI / module that does both $ ccoin <from> <to>, and when it's blank it prompts the user.

But we should also have a defaults, so when you run ccoin and it will always show your stored preferences.

$ ccoin
Which cryptocurrencies do you want?
  [x] Ethereum
  [ ] BitCoin

What currencies do you want to convert to?
 [x] American Dollar (USD)
 [ ] Japanese Yen (JPY)

Would you like to set default conversions? y/n

ETH/USD: 000.00

as well as: $ ccoin btc usd,eth,ltc --save || $ ccoin btc usd,eth,ltc --set-default

brh55 · 2017-06-12T16:09:51Z

The only downside is cryptocompare supports 100 different currencies (https://www.cryptocompare.com/api/data/coinlist/), so I can imagine that list being extremely long lol.

radiovisual · 2017-06-12T16:10:57Z

Ok cool. I like that the ability to save preferences or profiles.

So I could possibly do something like (assuming I had a profile saved called yen-stuff) :

$ ccoin -p yen-stuff

where -p is the flag to load a saved profile?

Or if I wanted to create a profile called yen-stuff:

$ ccoin btc,jpy, usd,jpy --save-as=yen-stuff

and then finally, the prompt could also ask if you want to save your settings as a profile:

Which cryptocurrencies do you want?
  [x] Ethereum
  [ ] BitCoin

What currencies do you want to convert to?
 [ ] American Dollar (USD)
 [x] Japanese Yen (JPY)

Do you want to save these settings? (Y/n) Y
What would you like to name this profile? yen-stuff
Ok, now you can run this profile with `$ ccoin -p yen-stuff`

Something like that?

radiovisual · 2017-06-12T16:12:01Z

The only downside is cryptocompare supports 100 different currencies hahaha

Yeah, baby-steps! 🍼 haha.

radiovisual · 2017-06-14T09:25:14Z

@brh55 has work begun on crypti/ccoin? I see the repo has been made, but nothing has been committed?

brh55 · 2017-06-14T14:32:09Z

@radiovisual Ah yes, I was going to commit the base repo from the generator but got side-tracked.

radiovisual · 2017-06-14T15:53:01Z

Ok, cool. I will monitor the progress and pitch in where needed. Are you going to make a separate module for the API calls to cryptocompare, or just hard-wire it into ccoin?

brh55 · 2017-06-14T16:48:43Z

I was thinking of making ccoin the core, but I could perhaps leverage this module?

radiovisual · 2017-06-14T17:32:08Z

yeah, I like that plan. I would use that module you pointed to since it already has the API we need. 😎

radiovisual added 3 commits June 11, 2017 09:22

switch popsicle to isomorphic fetch. Fixes crypti#3

258c70c

remove unused dependency

d3335de

Merge branch 'master' into fetch

f7f8082

radiovisual added 2 commits June 11, 2017 09:49

forgive spaces in api (to match cli)

8762497

Merge branch 'fetch' of https://github.com/radiovisual/eth-price into…

3b3f567

… fetch

radiovisual added 2 commits June 11, 2017 10:02

increase code coverage

1fa8145

add test files

a649039

radiovisual mentioned this pull request Jun 11, 2017

Popsicle + nock is breaking the tests? #3

Closed

brh55 requested changes Jun 11, 2017

View reviewed changes

remove redundant whitespace checks

ff8060f

tweak whitespace

3988026

brh55 approved these changes Jun 11, 2017

View reviewed changes

brh55 merged commit 9de10b1 into crypti:master Jun 11, 2017

radiovisual deleted the fetch branch June 12, 2017 07:22

Replace popsicle with isomorphic-fetch #4

Replace popsicle with isomorphic-fetch #4

Conversation

radiovisual commented Jun 11, 2017 • edited

coveralls commented Jun 11, 2017 • edited

coveralls commented Jun 11, 2017

coveralls commented Jun 11, 2017

coveralls commented Jun 11, 2017 • edited

brh55 left a comment

Choose a reason for hiding this comment

brh55 Jun 11, 2017

Choose a reason for hiding this comment

radiovisual Jun 11, 2017

Choose a reason for hiding this comment

brh55 Jun 11, 2017

Choose a reason for hiding this comment

coveralls commented Jun 11, 2017 • edited

coveralls commented Jun 11, 2017 • edited

radiovisual commented Jun 11, 2017 • edited

brh55 left a comment

Choose a reason for hiding this comment

brh55 commented Jun 11, 2017

radiovisual commented Jun 11, 2017

brh55 commented Jun 12, 2017 • edited

radiovisual commented Jun 12, 2017

brh55 commented Jun 12, 2017

radiovisual commented Jun 12, 2017

brh55 commented Jun 12, 2017 • edited

radiovisual commented Jun 12, 2017

radiovisual commented Jun 12, 2017

radiovisual commented Jun 12, 2017 • edited

brh55 commented Jun 12, 2017

radiovisual commented Jun 12, 2017

brh55 commented Jun 12, 2017

radiovisual commented Jun 12, 2017 • edited

radiovisual commented Jun 12, 2017

brh55 commented Jun 12, 2017 • edited

radiovisual commented Jun 12, 2017

brh55 commented Jun 12, 2017

radiovisual commented Jun 12, 2017

radiovisual commented Jun 12, 2017

brh55 commented Jun 12, 2017

brh55 commented Jun 12, 2017 • edited

radiovisual commented Jun 12, 2017 • edited

radiovisual commented Jun 12, 2017

radiovisual commented Jun 14, 2017

brh55 commented Jun 14, 2017

radiovisual commented Jun 14, 2017

brh55 commented Jun 14, 2017

radiovisual commented Jun 14, 2017

radiovisual commented Jun 11, 2017 •

edited

coveralls commented Jun 11, 2017 •

edited

coveralls commented Jun 11, 2017 •

edited

coveralls commented Jun 11, 2017 •

edited

coveralls commented Jun 11, 2017 •

edited

radiovisual commented Jun 11, 2017 •

edited

brh55 commented Jun 12, 2017 •

edited

brh55 commented Jun 12, 2017 •

edited

radiovisual commented Jun 12, 2017 •

edited

radiovisual commented Jun 12, 2017 •

edited

brh55 commented Jun 12, 2017 •

edited

brh55 commented Jun 12, 2017 •

edited

radiovisual commented Jun 12, 2017 •

edited