General opinion on the PR: I'm undecided whether that's an overall net-plus and don't see why it's needed, yet.

General opinion on the PR: I'm undecided whether that's an overall net-plus and don't see why it's needed, yet.

See, our AppConfig has some fields that don't map to end user configs, they are just there for keeping state (it shouldn't be this way). The eventual goal is to have exactly one field in AppConfig for each end-user config to make things consistent.

From a consistent AppConfig, we can derive more things. I try to make small PRs, so it's easier for the reviewers. This doesn't always make the benefits clear, but it doesn't mean we shouldn't try making the codebase more maintainable.

See, our AppConfig has some fields that don't map to end user configs, they are just there for keeping state (it shouldn't be this way).

I don't see any state in the db-tx fields. There might be a single config option and two fields, but that doesn't mean there is no clear mapping between them.

I don't see any state in the db-tx fields. There might be a single config option and two fields, but that doesn't mean there is no clear mapping between them

See in #3101 (comment):

data AppConfig = AppConfig
 { 
 ...
 , configDbTxAllowOverride :: Bool
 , configDbTxRollbackAll :: Bool
 ...
 }

Here marking these two configs as 'File or 'FileAndInDb is inconsistent with the fact that only one config actually exists but two are being used internally which is unnecessary and kind of naive.

Here marking these two configs as 'File or 'FileAndInDb is inconsistent with the fact that only one config actually exists but two are being used internally which is unnecessary and kind of naive.
, configDbTxAllowOverride :: Bool
, configDbTxRollbackAll :: Bool

I get the point that having two fields for a single config is inconsistent with all of the other ones but I still don't see the need for GADTs. I'd suggest to keep it the simplest way possible for now. I think we would just need a type like:

data PgrstConfig= {
 pcFileName :: Text
, pcEnvName :: Text
, pcInDbName :: Maybe Text
, pcDescription :: Text
...
}

pcInDbName being the only optional name, both file and env are always possible for all. As you can see with this is clear which are the sources of a config, without the use of GADTs.

Besides that I think there is a benefit for typing these values instead of being Bools:

data DbTxEnd
 = TxCommit
 | TxCommitAllowOverride
 | TxRollback
 | TxRollbackAllowOverride
 deriving (Eq)

That being we can deduplicate these descriptions later on:

Right now they're not consistent and unnecessarily separate. I think there's a future where we could generate the config docs automatically from our Config module.

@wolfgangwalther WDYT?

WDYT?

I think it's impossible to judge a small refactor PR like this without knowing the bigger picture where we want to get to. There are multiple different ideas on how to evolve the config module, but they need to be prototyped to see whether they actually work, where the limitations are etc.

It makes zero sense to start refactoring stuff before that.

I think it's impossible to judge a small refactor PR like this without knowing the bigger picture where we want to get to.

Assuming we want to deduplicate the descriptions as I mentioned above (which to me looks like an obvious benefit), how can we do it gradually?

Feeding the descriptiosn to the CLI seems possible now with interpolation. For docs, I'm thinking we would need a new CLI option like --dump-config-rst or --dump-config=rst which has the full output for:

It needs to be in one go since we need to ensure the alphabetical order of the configs.

Once that's done we could concatenate configuration.rst with this dump, maybe once postgrest-docs-build is invoked.

Now to do that in one big PR might be too complicated and hard.

Would a partial --dump-config=rst be reasonable? Then I can create a issue (with subtasks) with this idea, and we can merge PRs gradually.

how can we do it gradually?

Even PostgreSQL introduced its new AIO system gradually, not in one single commit (ref1, ref2, there are more). This for easier review. I think we should do that for big changes too, otherwise progress is too slow (plus new commits add the need for rebases and so on).

I'm not saying we need to introduce it all in one commit, far from it. But before we have something that we know we want to work towards, we can't judge whether our steps are in the right direction.

We need a prototype of how the solution should look like. And then we can commit steps towards it.

We need a prototype of how the solution should look like. And then we can commit steps towards it.

Agreed. Without a clear design, it is hard to know what might or might not work out.