-
Notifications
You must be signed in to change notification settings - Fork 222
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
Watch command added #88
base: master
Are you sure you want to change the base?
Conversation
The 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. |
@Saturn : Actually I had that in mind but the issue with that is if we have default as 120 seconds then |
@Saturn : I have integrated the comments mentioned by you in this updated PR. Pls have a look at it. |
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. |
@Saturn : Umm.. I tried looking out in general about the way to have |
I would prefer c992da4 be reverted before merging. I would have thought there would be a simpler way of accomplishing this but maybe there isn't. Keeping c992da4 as a reference. Maybe it is not necessary for I am going to leave it up to @architv to decide. |
Is this still active? If yes, I can fix the changes and we can get it merged. |
This PR is wrt #56 .