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
Collaborator

@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
Collaborator

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

@polarathene
Copy link
Collaborator Author

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

They're all on the master branch, this PR doesn't touch these files and the CI is catching them 🤷‍♂️

image

.map(|s| SourceType::Sync(s))

.map_err(|err| ConfigError::Foreign(err))

config-rs/src/value.rs

Lines 164 to 169 in d7c1656

Self::Table(ref table) => write!(f, "{{ {} }}", {
table
.iter()
.map(|(k, v)| format!("{} => {}, ", k, v))
.collect::<String>()
}),

config-rs/src/value.rs

Lines 170 to 172 in d7c1656

Self::Array(ref array) => write!(f, "{:?}", {
array.iter().map(|e| format!("{}, ", e)).collect::<String>()
}),

@matthiasbeyer
Copy link
Collaborator

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
Collaborator

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

@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 mehcode: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.

None yet

2 participants