search

mehcode / config-rs

⚙️ Layered configuration system for Rust applications (with strong support for 12-factor applications).
Apache License 2.0
2.57k stars 213 forks source link

Fix nested arrays (by reworking array handling) #465

Closed ijackson closed 11 months ago

ijackson commented 12 months ago

Fixes #464.

matthiasbeyer commented 11 months ago

Squashed the fixup commits for you and signed off with my own signoff because you didn't. I assume that you're alright with the signoff and its implications. Also, if you want, feel free to add your signoff to the commits as well!

matthiasbeyer commented 11 months ago

Meh. CI broke because a dependency of ours does no longer build with 1.66.0.

I'll file a PR to update MSRV, after that is merged please rebase this (and signoff your commits while you're at it! :laughing: ) please!

ijackson commented 11 months ago

Squashed the fixup commits for you and signed off with my own signoff because you didn't. I assume that you're alright with the signoff and its implications. Also, if you want, feel free to add your signoff to the commits as well!

I think by signoff you mean that you want my confirmation on the statements in the Developer Certificate of Origin , v1.1 presumably? I'm happy to confirm that all those statements apply to all of my contributions to config-rs, thanks.

You may want to copy DEVELOPER-CERTIFICATE into your tree. Otherwise it's slightly ambiguous what Signed-off-by means: what is the committer signing off on?

matthiasbeyer commented 11 months ago

I think by signoff you mean that you want my confirmation on the statements in the Developer Certificate of Origin , v1.1 presumably?

Yes!

You may want to copy DEVELOPER-CERTIFICATE into your tree. Otherwise it's slightly ambiguous what Signed-off-by means: what is the committer signing off on?

Ah, I thought that was the case. Good catch, I'll do this ASAP!

ijackson commented 11 months ago

I read in https://github.com/mehcode/config-rs/pull/483#issuecomment-1775535442 that you're not intending a release any time soon. That's awkward for us. We need this bugfix for a feature we want to release at least a preview of fairly soon. Our current workaround is not really suitable. If you don't intend to release soon with this fix, we may need to publish a forked crate to crates.io, which I would rather avoid!

matthiasbeyer commented 11 months ago

Do I read this correctly that this PR is what you'd need for your project? You're currently using 0.13.3 I assume?

Is there a chance that we can backport the fix to the release-0.13.x branch and publish 0.13.4 with the fix only? Would that be a solution for you?

I just did a quick test run. All the commits from this PR seem to apply cleanly to the release branch and tests run green. So I'd love to see a backport of this PR (after it goes in). I can also do the backport myself, if you want. I would say as long as CI is green, there are no breaking changes, so releasing a patch version should be fine, shouldn't it? :laughing: Please speak up if you think that these patches cannot be in a patch release for whatever reason though!

ijackson commented 11 months ago

Rebased, thanks.

Yes, a backport of this to the release branch would be perfect for us. I can make an MR for that right away if you like.

ijackson commented 11 months ago

Not sure I understand why the MSRV CI test is still failing. Did I rebase onto the wrong thing?

matthiasbeyer commented 11 months ago

I can make an MR for that right away if you like.

Please do! :heart: But after this PR went in and with cherry-pick -x so that the picked commits refer the commits in master!

matthiasbeyer commented 11 months ago

Not sure I understand why the MSRV CI test is still failing. Did I rebase onto the wrong thing?

You did not rebase on latest master...

ijackson commented 11 months ago

I think my push didn't take or something? Done now I think. Sorry.

matthiasbeyer commented 11 months ago

I think my push didn't take or something? Done now I think. Sorry.

Nah don't worry! :+1: