RepoSense icon indicating copy to clipboard operation
RepoSense copied to clipboard

Enforce argument priority order

Open fzdy1914 opened this issue 6 years ago • 19 comments

Step to reproduce: Enter the command that contains -formats field (java in the example) image However, the dashboard contains unrelated format image

fzdy1914 avatar Dec 13 '18 15:12 fzdy1914

The reason for this issue is that it will autoload the format in the config.json file. Do we need to set the isStandaloneConfigIgnored to true when using LocationsCliArguments? Or the level of format in the config.json file is meant to be higher than the one in command line & repo-config.csv ?

fzdy1914 avatar Dec 14 '18 03:12 fzdy1914

Can be joint into the discussion of #390

eugenepeh avatar Dec 14 '18 15:12 eugenepeh

@fzdy1914 is this issue still relevant?

damithc avatar May 16 '19 11:05 damithc

It is still valid. In https://github.com/reposense/RepoSense/issues/390, we have allowed the config in repo-config.csv to override values in standalone config. However, the config provided by command line still cannot override values in standalone config. The priority between repo-config.csv and the command line is not decided, too.

fzdy1914 avatar May 16 '19 12:05 fzdy1914

The current priority order is a bit odd. How about this? The normal priority order CLI > report config > standalone config This means report config files will override standalone config by default. However, the report config can allow the standalone config to override its values using the overridable: prefix. Is that a workable approach?

damithc avatar May 16 '19 14:05 damithc

The current priority order is a bit odd. How about this? The normal priority order CLI > report config > standalone config

The order makes sense.

This means report config files will override standalone config by default. However, the report config can allow the standalone config to override its values using the overridable: prefix.

Also workable. We can change the ignore standalone config to adopt standalone config which means we use overridable: prefix for all columns.

Is that a workable approach?

Under this circumstance, when CLI argument exists, it will override the same argument in both standalone config and report config file. Meanwhile, the meaning of --ignore should change to whether to only use the argument provided by CLI or also use the argument provided by standalone config or report config file which is not included in CLI. (i.e. ignoreGlobList and ignoreCommitList)

I hope my explanation is clear enough, lol.

fzdy1914 avatar May 16 '19 15:05 fzdy1914

sounds good. We can change --ignore-standalone-config to --apply-standalone-config/-a to mean apply standalone config. What do you think?

damithc avatar May 17 '19 05:05 damithc

sounds good. We can change --ignore-standalone-config to --apply-standalone-config/-a to mean apply standalone config. What do you think?

For this apply, do you mean that the rest field in standalone config can be used or the field can override the value in CLI?

fzdy1914 avatar May 17 '19 08:05 fzdy1914

The normal priority order CLI > report config > standalone config This means report config files will override standalone config by default. However, the report config can allow the standalone config to override its values using the overridable: prefix.

The order is better, however, wouldn't this means that if I were want to ignore a specific field in standalone config, I will need to fill up all the empty fields with the overridable: prefix?

eugenepeh avatar May 17 '19 12:05 eugenepeh

The order is better, however, wouldn't this means that if I were want to ignore a specific field in standalone config, I will need to fill up all the empty fields with the overridable: prefix?

Yes, I suppose so. It's inconvenient, but can tolerate. Not that difficult to copy-paste a value into a range of cells in excel. Any better alternatives?

damithc avatar May 17 '19 14:05 damithc

For this apply, do you mean that the rest field in standalone config can be used or the field can override the value in CLI?

It can mean the standalone overrides csv, not CLI. As -a is a CLI flag, it should not override CLI parameters (i.e., one flag in the command should not override other parts of the same command).

damithc avatar May 17 '19 14:05 damithc

Any better alternatives?

I am thinking of having 2 different mode, inherit and default. inherit will take the values from standalone config, while default uses the default value defined by the application. This is to define the action for the application to take, should the field in csv is empty.

When using the flag --inherit (which can also be -a), it will automatically use the values of the standalone config when the csv field is empty. While default can just be default, where application just the default value defined by the application, hence there is no need to have a dedicated flag for it. If the user truly wants a specific field to be empty, he can do it using a dedicated constant e.g. "" or empty.

How does that sound?

eugenepeh avatar May 18 '19 14:05 eugenepeh

We'd better define all possibilities.

  • --inherit
    • standalone missing, csv present:
    • standalone present, csv missing:
    • both absent:
    • both missing:
  • default
    • standalone missing, csv present:
    • standalone present, csv missing:
    • both absent:
    • both missing:

damithc avatar May 20 '19 13:05 damithc

In both cases, CSV has highest priority, hence:

  • --inherit
    • standalone missing, csv present: use CSV values, define blank fields with default value
    • standalone present, csv missing: inherits all standalone values
    • both present: use CSV values, empty fields will be obtained from standalone config.
    • both missing: use default value
  • default
    • standalone missing, csv present: use CSV values, define blank fields with default value
    • standalone present, csv missing: use default value
    • both present: use CSV values, define blank fields with default value
    • both missing: use default value

So in summary, --inherit shall use standalone config values whenever possible when it's not defined in CSV; CSV will still has the highest priority.

eugenepeh avatar May 22 '19 03:05 eugenepeh

Sounds good. Minor change given below. --inherit

  • both present: use CSV values, empty fields will be obtained from standalone config. use "" in csv file to use default instead of standalone

damithc avatar May 22 '19 14:05 damithc

To summarise the discussion above,

  1. CLI will have higher priority than standalone config --> If a user specifies the same argument on CLI and in standalone config, the value on the CLI takes effect. Currently, the only common argument in CLI and standalone config is formats.

  2. CLI will have higher priority than CSV Configs. Currently, the only common arguments between CLI and the CSV Configs are formats and ignore-standalone-config.

  3. CSV Configs will have higher priority than standalone config. Currently, the common arguments are formats, ignore commit list, ignore globs list and the author configurations. However, we can use the inherit mechanism to allow standalone config to fill in the empty fields in the config csv.

May I clarify the following:

  1. Is it right that, with the inherit / default mechanism, we no longer need the ignore standalone config column in repo-config.csv ? My reasoning: if we have N repos in repo-config.csv, and we wish to ignore the standalone config only for some of them, then we just need to fill out all the fields for these repositories and use inherit mode. This will allow the remaining repos to continue using their standalone configs.

  2. Is it right that these changes in priority do not affect the --ignore-standalone-config CLI option ? We still need a way for users to disable the standalone config when they are specifying repositories via --repo

anubh-v avatar Nov 02 '19 07:11 anubh-v

Good question

Is it right that, with the inherit / default mechanism, we no longer need the ignore standalone config column in repo-config.csv ? My reasoning: if we have N repos in repo-config.csv, and we wish to ignore the standalone config only for some of them, then we just need to fill out all the fields for these repositories and use inherit mode. This will allow the remaining repos to continue using their standalone configs.

Note that theres author configurations involve as well, that is something that need to be take care of as well.

Is it right that these changes in priority do not affect the --ignore-standalone-config CLI option ? We still need a way for users to disable the standalone config when they are specifying repositories via --repo

Yes

eugenepeh avatar Nov 02 '19 18:11 eugenepeh

Note that theres author configurations involve as well, that is something that need to be take care of as well.

Wouldn't it follow the same logic?

  • --inherit

    • standalone missing, author-config.csv present: Get author configurations from CSV. If author configurations for a particular repository are not present in the CSV, then use all authors within the date range.
    • standalone present, author-config.csv missing: Get author configurations from standalone config. If author configurations are not present in standalone, use all authors within the date range.
    • both present: Get author configurations from CSV. If author configurations for a particular repository are not present in CSV, use that repository's standalone config. If there are no author configurations in that repository's standalone config, use all authors within date range.
    • both missing: Use all authors within date range.
  • default

    • standalone missing, author-config.csv present: Get author configurations from CSV. If author configurations for a particular repository are not present, then use all authors within the date range.
    • standalone present, author-config.csv missing: Use all authors within the date range.
    • both present: Use author configurations in CSV. If author configurations for a particular repository are not present, use all authors within the date range.
    • both missing: Use all authors within the date range.

anubh-v avatar Nov 04 '19 12:11 anubh-v

Wouldn't it follow the same logic?

Do note that the complication here is, how are you going to enforce the use of all authors / override when the --inherit mode is present?

eugenepeh avatar Nov 04 '19 12:11 eugenepeh