I have thought this a little more and I think it would be better if we removed the default here and did another _register_formatter_defaults function. The documentation is pretty clear about what store_true is "supposed" to be used for:
'store_true' and 'store_false' - These are special cases of 'store_const' used for storing the values True and False respectively. In addition, they create default values of False and True respectively.

Just to be sure for future changes I think it makes more sense to honour the default values created by store_true and then parse the optional attribute. That also makes more sense for any outsider looking at this code for the first time.

What do you think?

I think I don't understand exactly what you propose. Could you adapt what we currently have ?

parser.add_argument(
    f"--{name}",
    action="store_true"
    default=not optional,
)
parser.add_argument(
    f"--{name}",
    action="store_false",
)    

It seems that if we remove the default value then we need additional parsing later on ? Can we guess if the value is user defined or simply the default ?

We'll need some sort of parsing later on anyway to make sure the default value actually gets into the Namespace object. I think it would be better to separate that parsing (and thus the parsing of the Formatter.optional attribute into its own function) than setting the default value here to a value that can be counter-intuitive to what the documentation says.

As for parsing and whether it is user defined I think this dependent on the order in which we parse.
I would suggest: defaults > config files > command line.

But right now we don't have to parse anything as the default override what store_const is doing (logically). If we create config file and need another layer we could use confuse to do something like this:

config: confuse.Configuration
config_default = config[name].get(confuse.Optional("bool"))
default = not optional if config_default is None else config_default 
parser.add_argument(
    f"--{name}",
    action="store_true"
    default=default 
)
parser.add_argument(
    f"--{name}",
    action="store_false",
) 

Also this is a part of the code that can almost never change as it's decoupled and generic, we don't have to touch it for every new formatter.

I have never worked with confuse so I don't know specifically what it does here.

I understand how the current format makes the code more efficient and achieve the same with less lines, but I think in the long run it only adds maintainability costs to the project: every peculiar thing a project does that is not "as expected" increases the cost for new contributors to understand what is happening. To me it makes much more sense to add two options using store_false and store_true and then parse a default value to store the correct value in dest. To me that feels more like the natural flow of argparse. However, if you're insistent on this we can keep your suggestion 😄

confuse is pretty nice let me show you with a confuse example :)

• Initializing the config : https://github.com/Pierre-Sassoulas/centralized-pre-commit-conf/blob/master/centralized_pre_commit_conf/main.py#L13
• confuse will search for the system configuration file in /home/user/.config/pre-commit-conf for Linux and
  %HOME%\AppData\Roaming\pre-commit-conf for Windows... Then if it does not find any it uses the default
• Default value are here : https://github.com/Pierre-Sassoulas/centralized-pre-commit-conf/blob/master/centralized_pre_commit_conf/config_default.yaml
• Or if something is overridden in the CLI call it uses the overridden value. The adapted argparse to do that using the confuse configuration : https://github.com/Pierre-Sassoulas/centralized-pre-commit-conf/blob/master/centralized_pre_commit_conf/parse_args.py#L21

So confuse handles the overriding of configuration (pretty intuitively, as it's what the lib does). There is a twitch for us : When do we recover our own default value from the formatter ? So we do not define a default value in our own yaml config ,so if confuse has something it must comes from the user. After that the following line: default = not optional if config_default is None else config_default actually does "if the configuration from confuse is not specified than the default comes from our formatter".

Hmm confuse looks nice, but is it really necessary for us? We don't really need to look for config files in usr directories (right now) and I think adding too many dependencies to such a simple auto-formatter might make it less compatible with other apps.

In the end I really like the approach of --formatter and --no-formatter (I didn't think of that myself), but I'd just like 1) not to overwrite the default value for a store_true option and 2) keep option adding and option parsing separate so we can easily iterate on them and know what is responsible for what.

This is quite a neat way to test changes occurred! Might use this myself for future tests as well!

Had to apply that locally anyway because the docstring was too long for flake8 :)