search

architv / soccer-cli

:soccer: Football scores for hackers. :computer: A command line interface for all the football scores.
MIT License
1.09k stars 221 forks source link

Watch command added #88

Open tusharmakkar08 opened 8 years ago

tusharmakkar08 commented 8 years ago

This PR is wrt https://github.com/architv/soccer-cli/issues/56 .

Saturn commented 8 years ago

The --watch option should have a typeclick.INT. If you provide a default value in the form of an int it will automatically assign it that type.

I think it should have a default value of 120 seconds or something. Which would mean you wouldn't have to explicitly state the type. So a win win situation.

Also in the help text you should specify that the units of time are in seconds.

The help text should also describe how it automatically 'refreshes' the live score output using the time period specified.

tusharmakkar08 commented 8 years ago

@Saturn : Actually I had that in mind but the issue with that is if we have default as 120 seconds then if watch won't work. It would be great if you can tell how to know when the option is actually called versus the passing of default value through click? A workaround which I was thinking was to have prompt=True there which has default=120 and if user types 0, it would be a single time thing. The help text would be updated accordingly. What do you think about it ?

tusharmakkar08 commented 8 years ago

@Saturn : I have integrated the comments mentioned by you in this updated PR. Pls have a look at it.

Saturn commented 8 years ago

Hi. I don't fully understand the changes. Are you sure by having a default value, it will cause this issue you speak of? Is this the only way around it?

I would have thought there would be simpler solution.

I do think the way input is currently being checked is not great in general. There must be a better way to determine what the user wants. A cleaner way.

tusharmakkar08 commented 8 years ago

@Saturn : Umm.. I tried looking out in general about the way to have default called out only when invocation is made but I was unable to find something related to it in click. We can use optparse or argparse (since they provide for such options) for this but then it would conflict with the click. The other option would be the prompt one, which I suggested before. However prompt wouldn't work exactly in the way we want whereas the current implementation (of passing context) is satisfying all the requirements. Let me know which way to proceed or if you come across a cleaner way to do that.

tusharmakkar08 commented 8 years ago

Any comments @architv @Saturn ?

Saturn commented 8 years ago

I would prefer c992da4b36251db81bc1d8a0aeee5c6d36f7348b be reverted before merging.

I would have thought there would be a simpler way of accomplishing this but maybe there isn't.

Keeping c992da4b36251db81bc1d8a0aeee5c6d36f7348b as a reference.

Maybe it is not necessary for --watch to act like a flag and have a default value if user does not supply one anyway.

I am going to leave it up to @architv to decide.

tusharmakkar08 commented 7 years ago

Hey @Saturn @architv : I have reverted back the old change and kept c992da4 as reference. Please have a look and merge it.

tusharmakkar08 commented 6 years ago

Is this still active? If yes, I can fix the changes and we can get it merged.