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.

bittrance · 2024-01-05T17:11:50Z

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

-        Ok(section.set(key.try_into().map_err(section::key::Error::from)?, new_value.into()))
+    ) -> Result<Option<Cow<'event, BStr>>, crate::file::set_raw_value::Error> {
+        let mut section = self.section_mut_or_create_new_filter(key.section_name(), key.subsection_name(), filter)?;
+        let key = key.name().to_owned().try_into().map_err(section::key::Error::from)?; // TODO Borrow, plz


This to_owned() is not optimal, but it is not obvious to me how to formulate the required lifetimes.

bittrance · 2024-01-05T17:17:07Z

gitoxide-core/src/repository/config.rs

+        match ((&input).section_name(), (&input).subsection_name()) {
+            (section, subsection) if !section.is_empty() => Filter {
+                name: section.into(),
+                subsection: subsection.map(ToOwned::to_owned),


I'm not sure exactly how the filter is used, so I'm not 100% sure this change retains the exact behavior of the filter in all corner cases.

bittrance · 2024-01-05T17:21:15Z

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

-        key: impl AsRef<str>,
-    ) -> Option<Cow<'_, BStr>> {
-        self.string_filter(section_name, subsection_name, key, &mut |_| true)
+    pub fn string<'a>(&self, key: impl Key<'a>) -> Option<Cow<'_, BStr>> {


Should we rename these lifetimes to `'lookup'?

bittrance · 2024-01-05T17:25:09Z

gix-config/src/traits.rs

+/// Parsed elements of a Git config key, like `remote.origin.url` or `core.bare`.
+pub trait Key<'a> {
+    /// The name of the section key, like `url` in `remote.origin.url`.
+    fn name(&self) -> &'a str;


Is name() to generic given that we implement it on &[b]str? .key_name() might be better?

bittrance · 2024-01-05T17:43:49Z

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

            .rev();
-        let key = section::Key(Cow::<BStr>::Borrowed(key.into()));
+        let key = section::Key(Cow::Borrowed(key.name().into())); // TODO Borrow, plz


Oops, this TODO is already addressed.

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!

Byron · 2024-03-03T09:17:38Z

gix-config/src/impls.rs

+
+use crate::traits::Key;
+
+impl Key for &String {


Now that I see it, the way Key is designed requires some duplicate work. In order to make any call to one of the *_by(section, subsection, key) methods, one will have to do extra work in order to get the subsection name, even though what one is really doing is to split a string across . two or three times.

That operation, along with the potential for failure, are already captured in parse::key(). Thus Key would probably be better named ParseKey while providing only one method to obtain a parse::Key.

Doing this would probably also do away with the duplication in implementation, as it then all boils down to getting a &BStr for parsing a key.

Byron · 2024-03-03T09:23:34Z

gix-config/src/tests.rs

+
+        assert_eq!("", "foo".section_name());
+        assert_eq!(None, "foo".subsection_name());
+        assert_eq!("foo", "foo".name());


Related to my earlier comment, this is a good example on where the API of Key isn't currently suitable to capture what a Key is. Top-level keys like "foo" are not valid, they need a section name, and empty section names are not permitted (nor are empty keys).

Thus I think it's important to be able to express a parsing failure or 'invalid key' as part of the Key trait (or ParseKey trait as proposed).

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

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

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

bittrance commented Jan 5, 2024

View reviewed changes

Byron requested changes Jan 5, 2024

View reviewed changes

bittrance force-pushed the config-key-take-2 branch 2 times, most recently from f1d7e94 to 00f7c28 Compare January 30, 2024 18:35

NobodyXu suggested changes Jan 30, 2024

View reviewed changes

gix-config/src/file/access/comfort.rs Outdated Show resolved Hide resolved

gix-config/src/file/access/comfort.rs Outdated Show resolved Hide resolved

gix-config/src/file/access/comfort.rs Outdated Show resolved Hide resolved

bittrance added 5 commits March 2, 2024 16:34

gix-config now uses a Key trait rather than Into<&BStr>

6281e1a

Update gix-config setters.

ba3bf65

Key trait docs.

096fd12

cargo fmt

b3556b2

Use .as_refs() in gix-config to avoid unnecessary monomorphization.

819d7eb

bittrance force-pushed the config-key-take-2 branch from 86e5f83 to 819d7eb Compare March 2, 2024 16:03

bittrance added 3 commits March 2, 2024 17:12

fixup! gix-config now uses a Key trait rather than Into<&BStr>

29e470b

fixup! cargo fmt

e5f815a

Fix docs for gix-config _by() methods.

e09a4ec

Byron reviewed Mar 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bittrance commented Jan 5, 2024

bittrance Jan 5, 2024

bittrance Jan 5, 2024

bittrance Jan 5, 2024

bittrance Jan 5, 2024

bittrance Jan 5, 2024

Byron left a comment

bittrance commented Jan 5, 2024

Byron commented Jan 5, 2024

Byron left a comment

Byron Mar 3, 2024

Byron Mar 3, 2024

Byron commented Apr 22, 2024

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

Are you sure you want to change the base?

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

Conversation

bittrance commented Jan 5, 2024

bittrance Jan 5, 2024

Choose a reason for hiding this comment

bittrance Jan 5, 2024

Choose a reason for hiding this comment

bittrance Jan 5, 2024

Choose a reason for hiding this comment

bittrance Jan 5, 2024

Choose a reason for hiding this comment

bittrance Jan 5, 2024

Choose a reason for hiding this comment

Byron left a comment

Choose a reason for hiding this comment

bittrance commented Jan 5, 2024

Byron commented Jan 5, 2024

Byron left a comment

Choose a reason for hiding this comment

Byron Mar 3, 2024

Choose a reason for hiding this comment

Byron Mar 3, 2024

Choose a reason for hiding this comment

Byron commented Apr 22, 2024

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

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