Parth-Vader / MobOff

A CLI to download, convert and send youtube videos to several devices using Pushbullet.
MIT License
48 stars 29 forks source link

Improved CLI support to pass API key & Device Id #62

Open rajula96reddy opened 6 years ago

rajula96reddy commented 6 years ago

We need to improve the CLI support for moboff initialise & moboff download in such a way that, we should be able to initialise using moboff initialize --api <key> --device <device_id> or moboff download --api <key> --device <device_id> --link <link>

This will come in handy in #60 & #61

ashwani99 commented 6 years ago

I would like to take this up

ashwani99 commented 6 years ago

I think the API key can be exported as an environment variable rather than providing it every time with moboff initialise or moboff download Also as --newdevice is already there, there might not be any need for --device What do you think? @rajula96reddy

Parth-Vader commented 6 years ago

@ashwani99 What are you saying exactly? Do you want to remove moboff initialise completely?

ashwani99 commented 6 years ago

@Parth-Vader No, moboff initialise will be there. I mean to say that moboff can read the API key from the environment variable. So the task of moboff initialise becomes only to set the preferred device and path to download. The benefit of this will be that users will not have to run moboff initialise again if they want to change just the API key. Also, there is no need of --device option as --newdevice is already supported.

rajula96reddy commented 6 years ago

@ashwani99 @Parth-Vader Sorry, I have been busy over the weekend and I couldn't participate in the discussion. @ashwani99 I think we should either have support to send the API & device through moboff initialise or pass both of them as environmental variables. But if you think about it, let's say we passed the API as Env variable and initialise with a default device say 0, the moboff initialise is interactive and we need to enter the Env variables only when it is prompted, which AFAIK is not easy. Which brings us to the question of 'Can we pass them as arguments?', which as you know is not currently there and needs to be implemented. Also, --newdevice is not an argument for initialise and can only be used with download given we have initialised .

ashwani99 commented 6 years ago

But if you think about it, let's say we passed the API as Env variable and initialise with a default device say 0, the moboff initialise is interactive and we need to enter the Env variables only when it is prompted, which AFAIK is not easy.

@rajula96reddy I think you misunderstood me here. I meant the user can export the api key as $ export PUSHBULLET_API_KEY=<key>. Now the program can read the api key from the environment value itself, it doesn't need to ask everytime moboff initialise is ran. Coming to moboff initialise it will display the availalbe devices and will ask us to set default device and also the download path.

The flow will be something like this:

# setting API key
$ export PUSHBULLET_API_KEY=<key>

# choosing device
$ moboff initialise
1 : device 1
2 : device 2
2
Please Select a directory for store the Downloads
The music would be downloaded to <chosen-path>
Now you can run `moboff download` :)

# setting a new default device
$ moboff download --newdevice 1 <link>
rajula96reddy commented 6 years ago

@ashwani99 Sorry! My bad. Yes! This is a good idea. I think we might have to change the core parts of the code, so I would say let's ask @Parth-Vader's opinion on this. @Parth-Vader can @ashwani99 go ahead make these changes?

Parth-Vader commented 6 years ago

@ashwani99 I will tell you why I prefer how the current moboff initialise.

You set the API key (which is meant to not change a lot), and it gets stored in a config file. It's the same as saving it in the environment value since the process of updating both is the same.

It's also a good idea to set a default device, but as an additional feature, the user can provide a new device if he wants to: this is also supposed to be rare for the user.

I agreed that moboff initialise --api <key> --device DEVICE could also be added, but I would prefer it to still be interactive. (No average user wants to pass options like that).

Do note that the name initialise was chosen because it is required to run only once.

ashwani99 commented 6 years ago

@Parth-Vader Thanks for the use-case overview. Do you still want --api and --device options to be added? If not, please consider closing this issue.

Parth-Vader commented 6 years ago

I would prefer closing this @ashwani99 @rajula96reddy Are you guys convinced?