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

📝 define MSRV #249

Merged
merged 16 commits into from
Oct 11, 2023
Merged

📝 define MSRV #249

merged 16 commits into from
Oct 11, 2023

Conversation

kevinmatthes
Copy link
Collaborator

As discussed in #248, it might be worth discussing whether this project should specify a certain Rust version as the Minimal Supported Rust Version (MSRV). This PR adds the basic setup for the current stable Rust version (v1.72.0):

  • configuration for Clippy
  • configuration for Cargo
  • a badge for the README for documentation purposes

I would like to discuss the following questions:

  • Should this project have an explicit MSRV specified somewhere; @ttytm?
  • Which Rust version should be used as MSRV?
  • Should there be a badge in the README stating the configured MSRV? If so, which colour shall it have?
  • How shall the MSRV be updated automatically? (This works with Renovate, I just need to know the period of time to pass before the next MSRV is suggested.)

@kevinmatthes kevinmatthes requested a review from ttytm as a code owner September 19, 2023 20:55
@kevinmatthes kevinmatthes self-assigned this Sep 19, 2023
@github-actions
Copy link

github-actions bot commented Sep 19, 2023

Filename                              Stmts    Miss  Cover    Missing
----------------------------------  -------  ------  -------  ------------------------------------------------------------
src/modules/api.rs                       24      13  45.83%   26-29, 38, 49-88
src/modules/args.rs                       4       4  0.00%    91-95
src/modules/config.rs                    32      26  18.75%   52-97
src/modules/forecast.rs                  20       6  70.00%   7-8, 24-25, 27-28
src/modules/localization.rs             108      44  59.26%   162-203, 230-265
src/modules/location.rs                  55      28  49.09%   25-29, 59-63, 75, 89, 95-104, 109, 118-126, 131-132, 142-143
src/modules/params.rs                    67      67  0.00%    24-137
src/modules/units.rs                     13       0  100.00%
src/modules/weather.rs                   25      25  0.00%    83-148
src/main.rs                              17      17  0.00%    26-49
src/modules/display/border.rs            47      47  0.00%    17-106
src/modules/display/current.rs          124     124  0.00%    37-243
src/modules/display/day.rs               92      92  0.00%    29-164
src/modules/display/graph.rs            175     170  2.86%    65-368
src/modules/display/gui_config.rs        15      12  20.00%   46-61
src/modules/display/historical.rs        98      98  0.00%    35-201
src/modules/display/hourly.rs           211     211  0.00%    40-396
src/modules/display/product.rs           21      21  0.00%    24-64
src/modules/display/utils.rs             46      46  0.00%    6-85
src/modules/display/weathercode.rs       34      34  0.00%    11-46
src/modules/display/week.rs              68      68  0.00%    30-137
src/modules/display/wind.rs              22      22  0.00%    17-42
TOTAL                                  1318    1175  10.85%

@github-actions
Copy link

github-actions bot commented Sep 19, 2023

Test Results

  3 files    3 suites   3s ⏱️
  9 tests   9 ✔️ 0 💤 0
27 runs  27 ✔️ 0 💤 0

Results for commit 03b6637.

♻️ This comment has been updated with latest results.

@kevinmatthes
Copy link
Collaborator Author

I just tested whether there is a default colour for the MSRV badge; there is none. So, we need to decide for / agree on a colour. The submitted suggestion uses "brightgreen" but I also saw "orange" and "green" (more yellow than green, in my opinion). If the colours are the same as for the other badges, "blue" and "violet" should work, as well.

@ttytm
Copy link
Owner

ttytm commented Sep 20, 2023

I'm currently on vacation. I'll look into it when a free moment opens up 👍

README.md Outdated Show resolved Hide resolved
.clippy.toml Outdated Show resolved Hide resolved
@ttytm ttytm mentioned this pull request Oct 10, 2023
@github-actions
Copy link

github-actions bot commented Oct 10, 2023

The MSRV will not change.

@kevinmatthes
Copy link
Collaborator Author

At least does the MSRV checking workflow work -- despite the contained stylistic issues!

@kevinmatthes kevinmatthes marked this pull request as draft October 10, 2023 21:04
@kevinmatthes
Copy link
Collaborator Author

I am sorry, the workflow for the MSRV check does not work as expected. I will remove it for now and add it in a separate PR.

@ttytm
Copy link
Owner

ttytm commented Oct 10, 2023

The check can fit as part of the lint workflow as msrv job. It should be possible to give permission on per job basis.

@kevinmatthes kevinmatthes marked this pull request as ready for review October 10, 2023 21:39
@ttytm
Copy link
Owner

ttytm commented Oct 10, 2023

Thanks for the work @kevinmatthes !

@kevinmatthes kevinmatthes merged commit 4d62092 into main Oct 11, 2023
13 checks passed
@kevinmatthes kevinmatthes deleted the documentation/msrv branch October 11, 2023 02:05
@ttytm
Copy link
Owner

ttytm commented Oct 11, 2023

To further simplify it, a failing CI step would suffice. Something like this might work.

cargo msrv list 2> versions.txt
new="$(egrep [[:digit:]]+\.[[:digit:]]+\.[[:digit:]]+ versions.txt | tail -n 1 | cut -d \  -f 2)"
current=$(cat .clippy.toml | cut -d \" -f 2)
if [ "$new" != "$current" ]; then
	echo "MSRV missmatch: $new != $current"
	exit 1
fi

Seeing a CI step failing and checking the output if necessary is more convenient than adding comments to the PR. It will be a clear enough indicator and would keep the PR cleaner.

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