serokell/xrefcheck

Make `ignoreRefs` a required parameter

Closed this issue · 5 comments

dcastro commented

Clarification and motivation

The ignoreRefs parameter, in the config file, is currently optional. This is in stark contrast with all the other parameters, all of which are required. This is an inconsistency.

Furthermore, I don't see a reason for ignoreRefs to be optional. A config file is automatically generated by the dump-config command, which already contains this field. So it's not like it saves the user any keystrokes to make this parameter optional.

Acceptance criteria

The ignoreRefs parameter is required.

Martoon-00 commented

Yeah, that was only for backward-compatibility purposes, generally, I think all options should be mandatory.

dcastro commented

that was only for backward-compatibility purposes

Oh, that's a really good point though!

I'm reconsidering this now. As we release more and more versions, if we always make new options required, how will users migrate their repo to the new xrefcheck version?

I can think of these options:

  1. Read the release notes, see if any options were added, manually add them to their existing config file.
    • I don't realistically expect all users to go and read release notes lol
  2. Run xrefcheck, see if it complains about a missing config option, manually add it, run xrefcheck again to see if it complains about another option, manually add it, etc, until xrefcheck stops complaining
    • I know developers have a high pain threshold 😆, but this is not a good user experience.
  3. Delete their config file, run xrefcheck dump-config, copy old keys to the new file.
    • Not ideal either

So, on second thought, making all options optional is starting to make sense. They all have default values anyway - so, what difference does it make to us whether an option is declared in the config file or not?

Making them optional would remove all effort from migrating to a newer xrefcheck version (at least in regards to newly introduced config options).

What do you think? Do you agree with making them all optional?

  • 😄1
dcastro commented

Here's another argument for making everything optional:

Say a user runs xrefcheck dump-config, and then changes some default value to some custom value (e.g. they change defaultRetryAfter: 30s to defaultRetryAfter: 90s).

But then they decide to use the default value after all, but they don't remember what it was. How do you go back to using a default value for they config option? I think the only way to do that would be to run xrefcheck dump-config again to go back to using the default value (which, again, is not a good user experience).

If we make everything optional, then the user can simply delete that line from the config, problem solved.

Martoon-00 commented

Those are fair arguments! I now agree that most of the options are better be made optional, this should improve UX.

Probably the only option I would leave mandatory - flavor one, as it affects correctness and the user must make a choice here. Would it make sense?

dcastro commented

Probably the only option I would leave mandatory - flavor one, as it affects correctness and the user must make a choice here. Would it make sense?

Yup, agreed. @Sereja313 can you please create an issue for this (and assign it to milestone 0.2.2)?

  • 👍1