Conversation
mre
force-pushed
the
feat-1930
branch
2 times, most recently
from
677c5fe to
752a50a
Compare
Member
Author
|
Addressed all comments. (Thanks, Thomas!) Also added a test which verifies that if the user points directly to a configuration file that doesn't contain a valid lychee section (like a basic Any thoughts? |
Member
That's great, thank you! Now I just think that pyproject still falls back to the default config and behaves differently, we could update that if possible and add that to the new test, which currently only covers package.json. |
katrinafyi
reviewed
Mar 27, 2026
Member
katrinafyi
left a comment
katrinafyi
left a comment
There was a problem hiding this comment.
Splitting it up into traits and loader module seems reasonable. Some comments.
This PR resolves #1930 by adding support for loading configuration from pyproject.toml and Cargo.toml. I've also decided to add package.json to the mix, as this is a very common way to configure tools in the JavaScript ecosystem. While implementing this, I realized the config module had grown out of proportion, which caused me to split it up into logical submodules to keep the core high-level structs clean. To handle the different config formats, I introduced a ConfigLoader trait. I went through a few iterations with this. Initially, I thought about a simple, imperative approach where we just attempt to strictly deserialize the entire file and return an option. However, because our config uses strict validation (denying unknown fields), a single typo by the user would cause the parsing to fail, and we would silently skip the file instead of telling them about the typo. To fix this, I split the trait into is_match and load. The is_match method does a lightweight check just to see if the specific lychee section exists in the file. If it does, we pass it to load, which enforces strict schema deserialization and properly bubbles up validation errors so the user knows what went wrong. Implementation-wise, I deliberately avoided heavyweight dependencies like the cargo_toml crate, which builds massive ASTs for the entire manifest. Instead, we use minimal, custom envelope structs with serde to extract only the lychee blocks. This keeps our binary size small and compilation fast. I also made sure that Cargo package metadata takes precedence over workspace metadata if both are present. Finally, I added comprehensive tests for each loader to ensure that valid blocks are mapped correctly, missing keys are ignored, precedence works as intended, and malformed schemas produce the expected errors.
- Consolidate config loaders into a single constant - Simplify Cargo.toml loader matching logic - Make config sections in pyproject.toml and package.json required - Improve error reporting when config section is missing - Update tests to reflect stricter config matching
- Replace separate is_match/load methods with single load() returning ConfigMatch enum - Use consistent .context() error handling instead of mixed approaches - Remove unwrap_or_default() fallback in pyproject.toml loader - Extend tests to cover missing lychee sections in all config file types - Fix clippy large_enum_variant warning by boxing Config
- Replace separate is_match/load methods with single load() returning ConfigMatch enum - Use consistent .context() error handling instead of mixed approaches - Remove unwrap_or_default() fallback in pyproject.toml loader - Extend tests to cover missing lychee sections in all config file types - Fix clippy large_enum_variant warning by boxing Config
thomas-zahner
approved these changes
Mar 31, 2026
katrinafyi
approved these changes
Mar 31, 2026
replaces workspace metadata
Member
Author
|
This was fun! The code turned out much better than my initial attempt thanks to both of your feedback. Much appreciated. I will go ahead and merge this. :) |
This was referenced Apr 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR resolves #1930 by adding support for loading configuration from pyproject.toml and Cargo.toml. I've also decided to add package.json to the mix, as this is a very common way to configure tools in the JavaScript ecosystem.
While implementing this, I realized the config module had grown out of proportion, which caused me to split it up into logical submodules to keep the core high-level structs clean. (Sorry if that PR got a bit large due to that. I'm mostly just moving stuff around and adding the trait + impls.)
To handle the different config formats, I introduced a ConfigLoader trait. I went through a few iterations with this. Initially, I thought about a simple, imperative approach where we just attempt to strictly deserialize the entire file and return an option. However, because our config uses strict validation (denying unknown fields), a single typo by the user would cause the parsing to fail, and we would silently skip the file instead of telling them about the typo.
To fix this, I split the trait into is_match and load. The is_match method does a lightweight check just to see if the specific lychee section exists in the file. If it does, we pass it to load, which enforces strict schema deserialization and properly bubbles up validation errors so the user knows what went wrong.
Implementation-wise, I deliberately avoided heavyweight dependencies like the cargo_toml crate, which builds massive ASTs for the entire manifest. Instead, we use minimal, custom envelope structs with serde to extract only the lychee blocks. This keeps our binary size small and compilation fast. I also made sure that Cargo package metadata takes precedence over workspace metadata if both are present.
Finally, I added some tests for each loader to ensure invariants (valid blocks are mapped correctly, missing keys are ignored, precedence works as intended, and malformed schemas produce the expected errors).