Apply default values when no value is passed #155

YoungMind1 · 2023-03-24T13:56:05Z

When no values specified, a notification is created with default values, this removes the need for a default flag.

24seconds · 2023-03-24T14:06:30Z

Hello @YoungMind1 , thank you for writing the PR!
[Suggestion]
How about supporting both option?

As-is

When the default option is given, then the pomodoro app creates default notification (25min work, 5min break)

> pomodoro create --default

To-be

Pomodoro supports two options - giving option explicitly and giving nothing. Then the pomodoro app created default notification. I'm not sure it is feasible but could you review this option? Thank you!

> pomodoro create
> pomodoro create --default

YoungMind1 · 2023-03-24T14:53:41Z

@24seconds By changing the default values of work_time and break_time the need of checking == 0 disappears.

Then we can do create or create --default.
I do think this is better, because then I can create notifications like create -w 40 (and break time defaults to 5 minute), which is better IMO.

24seconds · 2023-03-25T06:28:38Z

Oh.. good point @YoungMind1 . we should consider the case you mentioned.

> pomodoro create -w 40

As of now this is for 40mins work and 0mins break.

Two cases

So now I see there are two needs.

One is using zero values as zero (work zero value is 0min and break zero value is zero).
The other one is using zero values as non zero (work zero value is 25mins and break zero value is 5mins)

Ah, we also need to handle --default.

Suggestion

To meet those needs and not to make breaking change to this app, how about giving an option to users? With utilizing the --config option I think we can do this.
We need two things to cover.

Handling --default
Handling --work x and --break x (one of two are not given case)

Let say we now provide 4 config options.

work_time_default_value
break_time_default_value
work_time_zero_value
break_time_zero_value
When the user typed pomodoro create or pomodoro create --default then the first two configs would be used.

When the user typed pomodoro create -w 40 then the break time would be set as break_time_zeo_value.

When the user typed pomodoro create -b 10 then the break time would be set as work_time_zeo_value.

Example

For example user creates a json file like this. Let say this file name as config.json

{
  "work_time_default_value": 50,
  "break_time_default_value": 10,
  "work_time_zero_value": 25,
  "break_time_zero_value": 5
}

Then user initialize pomodoro app with config like below.

> pomodoro --config config.json

After initializing with config, then the commands will work like this

# this creates a 50mins work and 10mins break notification
> pomodoro create
> pomodoro create --default

# this creats a 40mins work and 5mins break
> pomodor create -w 40

# this creats a 25mins work and 10mins break
> pomodor create -b 10

# this creates a 30mins work and 5mins break. Because user specifies both work time and break time explicitly, zero values are not used.
> pomdoro create -w 30 -b 5

And to keep the current behavior, when the user initialize the app without config, then think of it as initializing app with this config.

{
  "work_time_default_value": 25,
  "break_time_default_value": 5,
  "work_time_zero_value": 0,
  "break_time_zero_value": 0
}

What do you think?

YoungMind1 · 2023-03-25T07:40:48Z

@24seconds Thank for the reply.

TBH, I don't like the idea of a set of defaults for "default" and another set for zero values.
usually the user is going to use the default values(that is why they are called default) if he/she wants to have minimum(zero) value for either of the times(work or break). he/she has to explicitly mention them. this is a rare case IMO and we should not cover it.

So, what is your opinion, it is you call(you are the boss :) ).

I like the idea of the config file, I will implement it

24seconds · 2023-03-27T01:36:33Z

@YoungMind1 Thank you for the comment. Yes.. I also think handling the --default is not natural.
Then how about this?

Do not touch pomodoro create --default. It will create a 25mins work and 5mins break notification
Support pomodoro create option. This will work same as pomodoro create --default
Provide two configurations - work_time_zero_value and break_time_zero_value. Those configurations will work as I commented above

I think this is more clear than my previous suggestion. What do you think?

YoungMind1 · 2023-03-27T20:18:20Z

@24seconds What about only supporting break_time and work_time ?

create --default and create are the same
when not mentioning break time or work time, the default values are taken.
no zero values at all is supported, you have to either manually set them in command, or set them as default.

if removing the zero values is not possible, then I think the best thing is to support all of them(4 values in the json file).

thanks for being open to new ideas and changes.

24seconds · 2023-03-28T03:23:47Z

@YoungMind1 Oh yes. What I'm saying and what you are saying are same. Maybe the term zero value makes you confused. In the golang, zero value means

Variables declared without an explicit initial value are given their zero value.
link: https://go.dev/tour/basics/12

So yes. Default value means same as zero value.

create --default and create are the same

Good

when not mentioning break time or work time, the default values are taken.
no zero values at all is supported, you have to either manually set them in command, or set them as default.

Good. Let's provide these options using configuration json.
Maybe the field name would be work_time_default_value and ``break_time_default_value`. This naming makes it more clear I guess. If these two fields are not specified, let's consider it as specifying those values as 0.

For example, if we initialize pomodoro without configuration, then it works as below.

# this creates a 25mins work and 5mins break notification. Untouchable.
> pomodoro create
> pomodoro create --default

# this creates a 40mins work and 0min break because `work_time_default_value ` and `break_time_default_value` are considered as 0 to keep the compatibility of previous pomodoro versions.
> pomodor create -w 40

# this creates a 0min work and 10mins break because `work_time_default_value ` and `break_time_default_value` are considered as 0
> pomodor create -b 10

# this creates a 30mins work and 5mins break. Because user specifies both work time and break time explicitly.
> pomdoro create -w 30 -b 5

If we initialize pomodoro app with configuration.json then it will work as below

{
  "work_time_default_value": 30,
  "break_time_default_value": 10
}

# this creates a 40mins work and 10mins break because  `break_time_zero_value` is specified as 10
> pomodor create -w 40

# this creates a 30mins work and 10mins break because `work_time_zero_value ` is specified as 30
> pomodor create -b 10

Please give me feedback about this @YoungMind1 .

nit

Maybe to express the time unit, the configuration option names are something like this?
work_time_default_value_in_minute, break_time_default_value_in_minute

24seconds · 2023-03-28T05:54:57Z

Hello, since this PR is merged #154, Could you rebase the main branch and work on top of it? There might be conflict I guess.

24seconds · 2023-03-30T01:34:47Z

Hello, @YoungMind1 . Is PR ready to be reviewed?

…le and rename the mock file

…ng logic

YoungMind1 · 2023-03-30T07:57:14Z

@24seconds PR is ready for review.

1 small change, instead of using 0 as the backup values(when config is not available), I am using the DEFAULT_WORK_TIME_VALUE and DEFAULT_BREAK_TIME_VALUE consts.

24seconds

I left some comment and suggestions. Could you take a look?

resources/test/mock_configuration.json

src/notification/mod.rs

24seconds · 2023-03-30T10:54:56Z

src/notification/mod.rs

-        assert!(notification.is_some());
-        let notification = notification.unwrap();
+        let (configuration, _) =
+            load_configuration(Some("./../../resources/test/mock_configuration.json")).unwrap();


[suggestion]
How about using CARGO_MANIFEST_DIR for the stable accessing?
https://doc.rust-lang.org/cargo/reference/environment-variables.html

24seconds · 2023-03-30T10:58:10Z

src/notification/mod.rs

[Suggestion]
How about writing test cases which covers our discussion?
To do this, I think mock_configuration should have distinct value (for example work default time 30, break default time 10)

I think we should cover scenarios below

pomodoro create --default -> notification (work:25, break:5)

pomodoro create -> notification (work:25, break:5)

pomodoro create -w 20 -> notification (work:20, break:10)

pomodoro create -b 20 -> notification (work:30, break: 20)

pomodoro create -w 20 -b 5 -> notification (work:20, break: 5)

This is not how it works currently, it will always create based on values defined in config file, if they are not present, then it will fallback to using const values

Added tests for all different combinations create command.

create with/without configuration file.

create --default with/without configuration file.

create -w x -b y with/without configuration file.

create -w x with/without configuration file.

create -b y with/without configuration file.

Yes, if the configuration is not given, it would do like this to consider backward compatibility.

pomodoro create --default -> notification (work:25, break:5)

pomodoro create -> notification (work:25, break:5)

pomodoro create -w 20 -> notification (work:20, break:0)

pomodoro create -b 20 -> notification (work:0, break: 20)

pomodoro create -w 20 -b 5 -> notification (work:20, break: 5)

Here is the current behavior example.

Yes, if the configuration is not given, it would do like this to consider backward compatibility.

I informed you of this behavior change here, I think this is more sensible, it is like overwriting the default values. if I run create -w x I am overwriting the default work_time value, but still using the default break_time.
⚠️ Note: here by default I mean the config file, and if it is not present the conts values(25, 5).

Thank you for the comment. I think it's not a small change. So we need to talk about this.

I think there are two aspects about not giving anything option.
For example, create -w x can be considered as

break time is not given. Therefore default value should be used. Default value can be specified by configuration json or can be used constant value - which is 5 minute.

break time is not given. Therefore break time is absent. Which can be considered it as zero value - which is 0 minute.

It is really hard problem. According to the cli guidelines link, it says about default like this.

Make the default the right thing for most users. Making things configurable is good, but most users are not going to find the right flag and remember to use it all the time (or alias it). If it’s not the default, you’re making the experience worse for most of your users.

So the point is which one is the right thing for most users?.
About this, I think 0minute is more sensible and you think falling back to default value is more sensible..

Hmmm.. I'm finding the which one is more sensible now. 🙇 This might take some time.

According to the pomodoro techinque in wikipedia, I think break time is part of pomodoro strategy. Let's do like this

create -w x -> create a notification (work: x, break: default value)

create -w x -b 0 -> create a notification (work:x, break: 0)

create -w 0 -b 0 -> wrong!

Plus, for the clear usage, I think updating the each option's explanation is needed.

What do you think @YoungMind1 ?

@24seconds Thanks for the comprehensive comments.
I am definitely going to check clig.dev, it seems informative.

What do you mean by wrong in number 3?

@YoungMind1 ah lets return error. As I remember the app returned error..?
I think 0 min work time and 0min break time notification does not make any sense

…ithout configuration file

24seconds · 2023-03-30T13:28:36Z

src/notification/mod.rs

+        //With configuration file
+
+        let (configuration, _) = load_configuration(Some(
+            &(env!("CARGO_MANIFEST_DIR").to_owned() + "/resources/test/mock_configuration.json"),


[nit comment]
It would be nice to use PathBuf when concatenating paths. I'm not sure this test would be passed in window. As I know (correct me if I'm wrong) window uses different character for separating the directory.

…with recent changes

24seconds · 2023-03-31T09:23:07Z

Hello @YoungMind1. If you are ready, please utilize review re-request feature 🙇

24seconds

Huge thank you for the contribution! Looks good to me.

And thank you for being patient while discussing the good ux and design. 🚀
Let's go!

24seconds · 2023-04-02T05:41:48Z

src/command/application.rs

@@ -115,21 +115,29 @@ pub(crate) fn add_args_for_create_subcommand(command: Command) -> Command {
    command
        .arg(
            Arg::new("work")
-                .help("The focus time. Unit is minutes")
+                .long_help("The focus time in minutes.


nice description!

24seconds · 2023-04-02T05:53:57Z

src/error.rs

@@ -38,6 +39,10 @@ impl fmt::Display for NotificationError {
                write!(f, "failed to get new notification: {}", e)
            }
            NotificationError::DeletionFail(msg) => write!(f, "{}", msg),
+            NotificationError::EmptyTimeValues => write!(


24seconds · 2023-04-02T06:18:45Z

src/notification/mod.rs

        assert_eq!(25, wt);
        assert_eq!(5, bt);
        assert_eq!(now, created_at);
    }
+
+    #[test]
+    fn test_create_new_notification_with_flags() {


[comment]
I see there are duplicate codes for testing. Maybe we can reduce it by writing it as table driven test cases.

related link: https://github.com/golang/go/wiki/TestComments#table-driven-tests-vs-multiple-test-functions

I did not find a way to implement test tables in rust. I could write some enums and structs in order to create a collection of test data on which we iterate and assert, but creating structs just for tests is a good idea IMO.

[Update]
I write a PR (#173). Maybe what you are thinking and what this PR tries to do are same I guess?

YoungMind1 · 2023-04-03T15:33:25Z

@24seconds
Thanks for your comprehensive and complete replies and feedback, I learned a lot.
Thanks for tolerating a newbie 😅

24seconds assigned YoungMind1 Mar 24, 2023

YoungMind1 added 2 commits March 29, 2023 17:58

Create the notification with default values when no value is specified

6cf1650

Use the default constants

66396b9

24seconds self-requested a review March 30, 2023 01:34

YoungMind1 added 3 commits March 30, 2023 11:20

Add work_time_default_value and break_time_default_value to config fi…

99d67e3

…le and rename the mock file

Remove the default values from the command definitinos.

4bca644

Pass down the configuration to create method and handle the overwriti…

a2be765

…ng logic

Format project

9a9084e

24seconds reviewed Mar 30, 2023

View reviewed changes

YoungMind1 added 5 commits March 30, 2023 14:56

Remove extra helping debuging prints

5a30b27

Use CARGO_MANIFEST_DIR in order to access mock_configuration file

f385fb3

Change values to be more distinct

6f14a33

Make configuration enum clonable

a386af4

Add create notification tests for all different combination of with/w…

47fe4d8

…ithout configuration file

24seconds reviewed Mar 30, 2023

View reviewed changes

YoungMind1 added 3 commits March 30, 2023 17:43

Use PathBuf for addressing

f167e1c

Change the help messages of create subcommands in order to be inline …

5dd6a56

…with recent changes

Change config flag's help message

b30fe89

Return error when work time and break time are zero

42e3fa5

YoungMind1 requested a review from 24seconds March 31, 2023 13:12

24seconds approved these changes Apr 2, 2023

View reviewed changes

24seconds reviewed Apr 2, 2023

View reviewed changes

24seconds merged commit c8d2565 into 24seconds:main Apr 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply default values when no value is passed #155

Apply default values when no value is passed #155

YoungMind1 commented Mar 24, 2023

24seconds commented Mar 24, 2023

YoungMind1 commented Mar 24, 2023 •

edited

Loading

24seconds commented Mar 25, 2023

YoungMind1 commented Mar 25, 2023

24seconds commented Mar 27, 2023

YoungMind1 commented Mar 27, 2023 •

edited

Loading

24seconds commented Mar 28, 2023

24seconds commented Mar 28, 2023

24seconds commented Mar 30, 2023

YoungMind1 commented Mar 30, 2023

24seconds left a comment

24seconds Mar 30, 2023

24seconds Mar 30, 2023

YoungMind1 Mar 30, 2023

YoungMind1 Mar 30, 2023

24seconds Mar 30, 2023

YoungMind1 Mar 30, 2023

24seconds Mar 30, 2023

24seconds Mar 30, 2023

YoungMind1 Mar 30, 2023

24seconds Mar 30, 2023 •

edited

Loading

24seconds Mar 30, 2023

24seconds commented Mar 31, 2023

24seconds left a comment

24seconds Apr 2, 2023

24seconds Apr 2, 2023

24seconds Apr 2, 2023

YoungMind1 Apr 3, 2023

24seconds Apr 5, 2023

YoungMind1 commented Apr 3, 2023

Apply default values when no value is passed #155

Apply default values when no value is passed #155

Conversation

YoungMind1 commented Mar 24, 2023

24seconds commented Mar 24, 2023

As-is

To-be

YoungMind1 commented Mar 24, 2023 • edited Loading

24seconds commented Mar 25, 2023

Two cases

Suggestion

Example

YoungMind1 commented Mar 25, 2023

24seconds commented Mar 27, 2023

YoungMind1 commented Mar 27, 2023 • edited Loading

24seconds commented Mar 28, 2023

nit

24seconds commented Mar 28, 2023

24seconds commented Mar 30, 2023

YoungMind1 commented Mar 30, 2023

24seconds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

24seconds Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

24seconds commented Mar 31, 2023

24seconds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YoungMind1 commented Apr 3, 2023

YoungMind1 commented Mar 24, 2023 •

edited

Loading

YoungMind1 commented Mar 27, 2023 •

edited

Loading

24seconds Mar 30, 2023 •

edited

Loading