search

mehcode / config-rs

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

chore: Update dependencies (`Cargo.toml`) #478

Closed polarathene closed 5 months ago

polarathene commented 8 months ago

Consistently omit patch version.

Package notes:

Outdated (not likely helpful) **UPDATE:** This information may not be that useful to anyone else, hence the collapsed section. It's mostly been superseded by what I learned when preparing https://github.com/mehcode/config-rs/pull/495 Other MSRV history since the March 2022 `warp` reference: - In March 2022 `config-rs` documented a version bump of [MSRV from 1.49 to 1.56](https://github.com/mehcode/config-rs/pull/304) (Nov 2021). - In March 2023 `config-rs` bumped [MSRV from 1.60 to 1.64](https://github.com/mehcode/config-rs/pull/426) (Sep 2022) due to dev-dependency `temp-env`. - In September 2023 `config-rs` [bumped MSRV again to 1.66](https://github.com/mehcode/config-rs/pull/455) (Dec 2022), with no documented reason for the requirement this time. You may want to update `Cargo.toml` to **communicate MSRV** better via [`rust-version`](https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field). - **EDIT:** A contributor [expressed reluctance to set an explicit `rust-version`](https://github.com/mehcode/config-rs/pull/483#issuecomment-1775534431). At the very least, the real MSRV required for published crate should be identified, perhaps via [`cargo-msrv`](https://github.com/foresterre/cargo-msrv)? --- AFAIK, the patch versions have been set often without bumping, and without any actual requirement to make that the minimum patch version. So they are not actually serving much purpose. Initially I misunderstood these to implicitly be `=` (fixed), hence the original motivation of the PR. As that's not the case, this does raise `lazy_static` to `1.4` as a minimum, which could be reverted. `lazy_static` last [released `1.4` in Aug 2019](https://github.com/rust-lang-nursery/lazy-static.rs/releases/tag/1.4.0), so I don't consider that a concern. **EDIT:** - A recent example with one of the projects dependencies releasing with a `major.minor` semver dependency of their own, and that dependency later pushing out a point release with a feature only stabilized since Rust `1.71` (July 2023), which leaked through to `config-rs` CI catching the failure (_but could just as easily slipped through like for `rust-ini` if the timing was a little later with the `ordered-multimap` crate patch release?_ 🤷‍♂️ ) - There was [another recent one with `ahash`](https://github.com/mehcode/config-rs/issues/491) too (_[upstream issue](https://github.com/tkaitchuck/aHash/issues/174)_), but since resolved. --- ~~Additionally, the `version` field is set to `0.13.1`, while crates reports `0.13.3`?~~ (_**EDIT:** `0.13.x` is maintained via a separate branch, [related PR raised](https://github.com/mehcode/config-rs/pull/495)_) Another possible update is the [`edition` to 2021](https://doc.rust-lang.org/stable/edition-guide/rust-2021/index.html) 🤷‍♂️
polarathene commented 8 months ago

@matthiasbeyer I've been granted maintainer rights to config-rs by @mehcode

After that burst of activity, I don't think I can continue to assist maintaining the project much. I've got too many commitments to handle already 😅

matthiasbeyer commented 8 months ago

Yes, I requested you to be added! I saw that you're doing a lot of stuff and I didn't want to slow you down by slow reviews and merges! So go ahead and do what you think is necessary!

matthiasbeyer commented 8 months ago

Ah, to amend on the above (and because it also came up elsewhere): I care a lot about clean commit history (as you probably already noted). That means, please don't do squash merges, not even for single-commit-PRs. And do not remove the gitlint CI job. I know it can be annoying to care about all the details to make it green, but IMHO it is worth it.

Besides that, feel free to take this project to new highs! Especially as I have only been a passive maintainer (except for the config-rs-ng stuff). :laughing:

polarathene commented 8 months ago

I care a lot about clean commit history (as you probably already noted). That means, please don't do squash merges, not even for single-commit-PRs.

Odd, I care about that too, and it's why I encourage squash merging as a default. I still use merge commit or rebase merge when it's appropriate (like retaining history across file renames, which Github otherwise doesn't track well).

For the type of contributors docker-mailserver attracts, it tends to work much better that we can iterate through a PR and review it, then control the history on our end. Often a squash works well for that and we rely more on the PR context. My own PRs there try to keep a clean commit history and commit messages providing context, I still prefer a squash merge to the primary branch.

My main gripe with merge commits is long-lived PRs (which isn't great but is a reality sometimes), unless these are rebased they'll interleave the commit history with other commits on the primary branch the PR is merged into, I really do not like that. A rebase prior to merging can address it, but that often doesn't work well when another maintainer or contributor has done a merge commit into the PR prior (it's not uncommon to see someone merge master branch into the PR instead of a rebase).

IIRC, the project was using merge commits prior for years, and it wasn't pleasant to trawl through since the prior maintainers did not do any QA on the PR commit history.

On the primary branch, I'm interested in a history that is easy to track changes and get context through git blame on when and why a change happened. I get that from squash merges just as well, and they're logical commits, a contribution is now typically a single commit when viewing a files history, IMO it's a far better experience when you actually need the history (assuming you can derive more context from a linked PR, otherwise I'd agree with avoiding squash commits).

And do not remove the gitlint CI job

Oh I wasn't planning on removing that. I can understand wanting the sign-off to be mandatory, but not so much the 80 char width.. that's a bit outdated with todays screens, even with a terminal view using a small portion of that width 🤷‍♂️ (besides many viewers can wrap text these days quite well).

In one project a similar limit lint was applied to markdown files I think which was an accessibility kind of issue. The intent was for the outdated reasoning of fixed line width, but was quite a hassle to write documentation for unless doing it as a post-process, while reading the existing source was likewise awkward when you had plenty more horizontal space, and when you had less you'd either need to scroll or get an even worse experience with word-wrap involved.

I could link to that as an example of where this practice is a negative impact, I rarely see a solid justification for it whenever I bring it up these days. If anything it discourages more detailed commit messages :(

I know it can be annoying to care about all the details to make it green, but IMHO it is worth it.

I'm fine with this, we value our test suite at docker-mailserver quite a bit. The tests and linting checks are extremely helpful, we recently caught a non-obvious test failure where two values were compared and failed a test but looked the same. The tests were comparing with LF line-endings, while the contributor (myself) had accidentally introduced CRLF. You'd think we'd have had a .gitattributes to prevent that 😅

I just checked and noticed config-rs lacks one... So it's possible any users on Windows could contribute files with CRLF in the commits. You won't be able to clean prior commit history for any existing CRLF if that's the case, but it can be normalized and from that point on avoid the concern. If you'd like I can take care of that, or you can refer to:

Besides that, feel free to take this project to new highs! Especially as I have only been a passive maintainer

I would if I could spare the time, but I'm already juggling quite a bit :(

My goal is to wrap up my contributions and leave the project, hopefully in a better state than I found it via some small improvements for maintenance like this PR description notes.

I'd like to avoid future commitments beyond that, docker-mailserver has taken a considerable amount of my time and I've been meaning to step down all year from that 😅 (or at least be much more passive maintainer)

polarathene commented 8 months ago

Another possible update is the edition to 2021 🤷‍♂️

For reference, in the recent PR I reviewed (PEM custom file format example), I added a suggestion to improve it which worked fine locally using the 2021 addition with an array iteration, but on 2018 required prepending vec! (the contributor had a more verbose typed workaround prior to that though).

polarathene commented 8 months ago

I could link to that as an example of where this practice is a negative impact

For the project, this was recently encountered, commit message snippet:

This struct seems to be a remnant of a previous approach to async.

It cannot be used, and the documentation was lies.  Eg:

warning: unresolved link to `AsyncConfigBuilder::build`
   --> src/builder.rs:116:43
    |
116 | /// To obtain a [`Config`] call [`build`](AsyncConfigBuilder::build) or [`build_cloned`](AsyncConfigBuilder::build_cloned)
    |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^ the struct `AsyncConfigBuilder` has no field or associated item named `build`
    |
    = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default

TRhere isn't a build method.  There are, in fact, no methods at all.

Reworked to satisfy CI:

This struct seems to be a remnant of a previous approach to async.
It cannot be used, and the documentation was lies.
There isn't a `build` method. There are, in fact, no methods at all.

The content maybe isn't what you'd want in the commit message, but it's an example of where length is important and line breaks isn't well suited as a workaround.

My own commits in the project are a bit more tame (eg: lengthy bulletpoints, one liner).

matthiasbeyer commented 5 months ago

Rebased and will merge if CI is green.