Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Use a common method in parsers to check root is a table #469

Merged

Conversation

polarathene
Copy link
Collaborator

Presently there is an approach implemented for JSON5, while other parsers have a TODO comment. I adapted the approach used for the Dhall config support and leveraged it as a common method for ValueKind.

AFAIK, the initial parser call for the text value into a from_* method will handle any earlier failure, while this ensures that parse() returns the expected map from ValueKind::Table. I think this satisfies the TODO?

Copy link
Member

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@polarathene You're pretty active here, thank you for your contributions! I want to note that I have been working on a complete overhaul of the config-rs codebase here: https://github.com/matthiasbeyer/config-rs-ng/ for some time, but have been stalled. Maybe you want to have a look?

@polarathene
Copy link
Collaborator Author

@polarathene You're pretty active here, thank you for your contributions!

You're welcome! It's not much but I try lend a hand when I can spare the time.

Maybe you want to have a look?

I would, but I have quite the backlog elsewhere to catchup on 😅 (I'm also not too experienced with Rust)

I just wanted to try pitch in some time to help with the two stale config features, and afterwards I figured these two recent PRs were small enough improvements to throw your way :)

I understand with things taking time and stalling, some of my own are dragging out by 1-2 years, some even longer 😝

@polarathene
Copy link
Collaborator Author

Clippy failures aren't related to this PR, how would you like them resolved?

@matthiasbeyer
Copy link
Member

Hm, with 1.70.0, these clippy failures do not appear on master for me? 👀

@matthiasbeyer
Copy link
Member

Ah, yes. I was using 1.70.0, but CI obviously runs 1.73.0.

I will push a fix in a few minutes!

@matthiasbeyer
Copy link
Member

After #471 is merged you can rebase for fixes to these issues.

Signed-off-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
@polarathene polarathene force-pushed the chore/common-parser-root-check branch from 095bb32 to a5e6e57 Compare October 7, 2023 04:06
@matthiasbeyer matthiasbeyer merged commit 55c464e into rust-cli:master Oct 7, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants