gersemi icon indicating copy to clipboard operation
gersemi copied to clipboard

Allow setting `--warnings-as-errors` in `.gersemirc`

Open Jacobfaib opened this issue 1 month ago • 5 comments

As title suggests, setting this in .gersemirc currently yields the following warning

/path/to/.gersemirc: these options are supported only through command line: warnings_as_errors

Jacobfaib avatar Dec 04 '25 14:12 Jacobfaib

Actually I guess reading over gersemi --help I now notice (emphasis mine):

control configuration: These arguments control how gersemi operates rather than how it formats source code. Values for these options are not read from configuration file. ...

I would actually extend the original ask to all options in control configuration. I can imagine --config would be tricky to implement (and --line-ranges rather useless) but the rest of the arguments seem appropriate to want to hardcode in a config file.

Jacobfaib avatar Dec 04 '25 14:12 Jacobfaib

[...] but the rest of the arguments seem appropriate to want to hardcode in a config file.

I don't agree with that assessment. It was like that some time ago (pre 0.17.0) but then I've realized that outcome configuration needs to be shared among project contributors to be effectively enforced however control configuration doesn't because each user might want to use it differently and they don't have to agree with other contributors at all.

Let's make a quick review:

  • --quiet: Debatable whether it makes sense to be shared, I can see both sides but I argue it should be up to a particular user.
  • --color: If you don't install colorama or simply don't want to use colored diff output (or even you don't want use --diff in the first place) why would project configuration enforce that?
  • --workers: so let's say you have big 32 core machine and you hardcode 32 while I have a poor 4 core computer, why should it be enforced? The only values that makes some kind of sense to be shared are either 1 or max (which is default btw). Let user decide that.
  • --cache: Rarely you want to disable cache, it usually means that there's a bug in gersemi you want to report, not particularly valuable to enforce that project-wide. Even if you take into consideration #86 how do you want to setup this value in a file for, let's say, Windows user and Linux user in a sane way that can be shared among those two?
  • --config: Tricky as you said, kind of recursive in nature.
  • --line-ranges: As above.
  • --warnings-as-errors: I have similar stance about that as with --quiet.

BlankSpruce avatar Dec 04 '25 15:12 BlankSpruce

The config file is mainly used by projects to set required options, yes, but also to set defaults. This is so that contributers can just run

$ gersemi src/

and know that they will get the same output on their machine as another. Otherwise, we would need to tell contributors "run gersemi, but don't forget to add --x --y --z".

In any case, all of these options the command-line still overrides the config file so allowing them in the config file wouldn't remove user choice, no?

how do you want to setup this value in a file for, let's say, Windows user and Linux user in a sane way that can be shared among those two?

The usual way this is done is to mandate one format in the config file and let pathlib.Path convert it to the platform-specific one later. For example, let's say we mandate unix:

cache: /path/to/file/../relative/path

then you would translate this like so

from pathlib import PurePosixPath, Path

cache_dir = read_config(...)
# The 2-step is important. We want to have PurePosixPath accurately interpret the unix path
# before transforming it to the platform-specific version in the Path() constructor
path = Path(PurePosixPath(cache_dir))

In the example below I'll use PureWindowsPath because I'm on a linux machine, but as you can see it properly splits stuff apart

>>> cache_dir = "/path/to/file/../relative/path"
>>> PureWindowPath(PurePosixPath(cache_dir)).parts
('\\', 'path', 'to', 'file', '..', 'relative', 'path')

Jacobfaib avatar Dec 04 '25 16:12 Jacobfaib

The trick with Path(PurePosixPath(...)) is a very nice example of "how to do that in implementation" but as you've noticed it imposes the limitation that only (unless I'm missing something obvious that isn't "evaluate environment variables") relative paths will make sense in that setup. While relative paths aren't bad per se, it's probably the most common with definitions and extensions, I imagine most use cases for cache is a single file used for all projects. Bear in mind that cache is used simply as a mean to not be unacceptably slow. If there are some use cases I'm not aware let's discuss that in #86.

In any case, all of these options the command-line still overrides the config file so allowing them in the config file wouldn't remove user choice, no?

It's not about "removal of user choice", it's about "why would configuration file enforce that". Just because we can do something doesn't mean we should. I acknowledge that pre 0.17.0 something along these lines was possible but I also think now that it was a mistake. Should default configuration filename be something different than .gersemirc? Most likely yes, rc part may suggest the whole runtime configuration is changeable there. I didn't change it since 0.17.0 because I didn't want to break existing workflows.

The only reason I can currently come up with why you care about sharing more options through .gersemirc is that you are unhappy about some defaults. You can either tell me about them (like cache in other issue) or you can work around that with at least these two ways:

  • use pre-commit hooks to facilitate usage of gersemi that's more suitable to your project and define your favourite options there
  • wrap gersemi in a helper script shared in your project

BlankSpruce avatar Dec 04 '25 16:12 BlankSpruce

that only (unless I'm missing something obvious that isn't "evaluate environment variables") relative paths will make sense in that setup

Hmmm absolute paths would work as well, no?

>>> PureWindowsPath(PurePosixPath("/absolute/path/to/file")).parts
('\\', 'absolute', 'path', 'to', 'file')
>>> PurePosixPath("/absolute/path/to/file").parts
('/', 'absolute', 'path', 'to', 'file')

I guess you couldn't attach a drive letter in the windows case (in such a way that posix understands it)... but it would seem silly to want to specifically put the cache on a particular drive that isn't the same one the repo is in currently.

I acknowledge that pre 0.17.0 something along these lines was possible but I also think now that it was a mistake.

Yeah, up to you of course on whether you want to implement this, I just found it strange that some options were settable only on command line. Usually it is the case that if a config file is supported, that all options are settable through both (barring any corner cases like --config).

Feel free to close the issue if this is not the direction you want to go. As you say, there are ways to work around the problem so it's not a blocking issue.

Jacobfaib avatar Dec 04 '25 17:12 Jacobfaib