Config access uses `impl Key` to allow type-safe keys #1236

bittrance · 2024-01-05T17:08:12Z

This PR changes the API of gix_config::File to use a trait (gix_config::Key) as key when requesting or setting configuration. This will allow gix to use its tree of structured and validating typed keys (gix::config::tree::sections::*) as keys directly, and also means that consumers of the gix crate to use type-safe config access. gix-config also provides an implementation for &str and &bstr so that constant strings can be used as keys. This is part of the work addressing #1125.

The PR is draft, as there remains various question marks, see specific comments in code below.

gix contains a lot of strings and format!() operations that could be changed to use type-safe keys, but that is left for a separate PR.

Tasks

initial refactor
get repository tests to work and remove TODOs around mutations (see as_config_key())
call key parameters value_name instead to be in sync with KeyRef::value_name, also see keys() methods
proper split into commits with breaking-change signalling and documentation of breaking changes

gix-config/src/file/access/raw.rs

gitoxide-core/src/repository/config.rs

gix-config/src/file/access/comfort.rs

gix-config/src/traits.rs

gix-config/src/file/access/raw.rs

Byron

Thanks for sharing your amazing progress!

Before I can look at anything in detail, I'd need the commits split up so that…

…the Key related changes in gix-config are in one commit, whose subject is prefixed with fix!: or feat!: . Please write the subject and body so they can go into the changelog.
…all other changes to adapt to this change can go into its own commit without any special message requirements.

There is also this one big request of only modifying the string_by_key based access with the new trait. After all, the natural way of accessing the API is by passing triplets of section, subsection and 'key' (definitely open to a better term here).

That way, only string_by_key() has to change at all and that change would probably be compatible to most existing uses depending on the available implementations of Key for &BStr for example. I don't think there is a need to break the world here, and hope this could even work without making any breaking change.

Is that feasible at all?

bittrance · 2024-01-05T20:12:48Z

…the Key related changes in gix-config are in one commit, whose subject is prefixed with fix!: or feat!: . Please write the subject and body so they can go into the changelog.

Is the purpose simply to have a feat! commit in the history? If so, I'd just split out adding the Key trait into a separate commit, no problem. The current commit looks like it does because I'm always striving for every commit to be viable (i.e. buildable and git-bisect:able) and still be meaningful.

There is also this one big request of only modifying the string_by_key based access with the new trait. After all, the natural way of accessing the API is by passing triplets of section, subsection and 'key' (definitely open to a better term here). That way, only string_by_key() has to change at all and that change would probably be compatible to most existing uses depending on the available implementations of Key for &BStr for example. I don't think there is a need to break the world here, and hope this could even work without making any breaking change.

I was thinking we should keep the API surface small by removing all the _by_key method so that only the impl Key versions remained (I prepared bittrance@c54bb3a for this purpose). My thought is that gix config API should encourage using type-safe access and I think config.string(format!("foo.{submod}.bar")) is more readable than config.string("foo", submod, "bar"), but I defer to your judgement on when it is worth making a breaking API change.

Byron · 2024-01-05T20:52:37Z

Is the purpose simply to have a feat! commit in the history? If so, I'd just split out adding the Key trait into a separate commit, no problem. The current commit looks like it does because I'm always striving for every commit to be viable (i.e. buildable and git-bisect:able) and still be meaningful.

No, the purpose is to have a changelog that applies to the gix-config crate exclusively - otherwise cargo smart-release would think there are breaking changes in all of the crates whose code was adjusted, and it will claim new features in their changelogs, too.

That takes priority over each commit being green on CI, it's all trade-offs.

I was thinking we should keep the API surface small by removing all the _by_key method so that only the impl Key versions remained (I prepared bittrance@c54bb3a for this purpose). My thought is that gix config API should encourage using type-safe access and I think config.string(format!("foo.{submod}.bar")) is more readable than config.string("foo", submod, "bar"), but I defer to your judgement on when it is worth making a breaking API change.

Thanks for sharing your thoughts.

For a plumbing crate, it would already be more than fine. However, it's exposed in gix and thus it needs to provide more support for a nicer API, but that shouldn't supersede the 'API that is natural for the data', i.e. triplets as it's without overhead.

But since you would be touching all of these methods anyway, let's encourage the use of the potentially typesafe and nicer API by making their methods the shorter ones. This means, for example, to keep File::string(key: impl Key) as is, but have a longer method that takes the triplet, like File::string_by(section, subsection, key). The _by suffix is an example but something that seems to work nicely, and it seems better than my initial thought _by_triplet() which is punishingly long.
Please do feel free to experiment (and bikeshed) with the suffix though.
Finally, this also means that string() would call string_by().

I hope all that makes sense, please do let me know about any concern you might have as well.

gix-config/src/file/access/comfort.rs

Byron

Thanks so much for the changes!

It's wonderful to see how the standard-API of gix-config can be enhanced by the Key trait. While looking at it more closely, I also noticed something else that I think can be fixed without too much overhead.

But if that work should ever appear overwhelming or boring because the API surface is quite massive, please let me know and I can help doing the refactoring. The last thing I'd want is for you to get exhausted due to all these comments of mine 😅.

Just let me know how I can help and I will try to make it happen.
Thanks again!

gix-config/src/impls.rs

gix-config/src/tests.rs

Byron · 2024-04-22T07:10:20Z

As a note, I plan to finish this PR myself once I get a chance.

Conflicts --------- gix-config/src/file/init/from_env.rs gix-config/src/parse/mod.rs gix/src/config/cache/access.rs gix/src/config/cache/incubate.rs gix/src/config/overrides.rs gix/src/repository/index.rs

- reorganize `Key` trait implementation - make it possible to use it as reference (and thus re-use the impl when passing it to another function that takes a `Key` - make it less wasteful to obtain the parts of a Key - where possible, rename methods to `*_by`, while the original method is replaced by one that uses a `key: impl AsKey` instead. That way, the methods that are easiest to call are the shortest ones, while still providing the ones that take tuples like before.

Byron · 2024-06-21T18:52:55Z

The content of this PR was merged to main, but I forgot to push before changing the commit message, hence GitHub doesn't see the merge happened.

Thanks again for getting this started!

bittrance changed the title ~~Config access uses impl Key to allow structured keys~~ Config access uses impl Key to allow type-safe keys Jan 5, 2024