halloy icon indicating copy to clipboard operation
halloy copied to clipboard

PRs quality improvement

Open KaiKorla opened this issue 9 months ago • 7 comments

This PR should helps to improve the quality in development and future PRs:

  • Adding a starting .editorconfig
  • Adding MegaLinter as additional PR check. It includes four checks for now:
    • EditorConfig, which checks all (applicable) files against the .editorconfig
    • Rust Clippy, which using Clippy to check Rust code files
    • CSpell, which checks spelling in Markdown files
    • Lychee, a link checker written in Rust, which checks local links in Markdown files

The GH-workflow will add a comment to each PR with the status of the checks. For example

image

Hope this helps to improve quality before a PR is merged. Feedback welcome!

PR Checklist

  • [x] fix typos in documentation #920
  • [x] fix additional typos #924
  • [x] fix CSPELL errors / update word list in cspell.json
  • [x] fix (final) line endings / newline
  • [x] fix line length
  • [x] fix Clippy

KaiKorla avatar Apr 14 '25 20:04 KaiKorla

Needs #920 be merged first + discussing the errors (especially the setting "max_line_length" in .editorconfig).

KaiKorla avatar Apr 14 '25 20:04 KaiKorla

Remember to rebase :)

casperstorm avatar Apr 16 '25 16:04 casperstorm

@casperstorm @tarkah what's your favorite line length? Rust coding conventions says 100 (https://rustc-dev-guide.rust-lang.org/conventions.html#line-length). If we use this also, we have to fix some code files (https://github.com/squidowl/halloy/actions/runs/14496743951/job/40666303833?pr=921#step:4:98)

KaiKorla avatar Apr 16 '25 16:04 KaiKorla

I'm fine with 100.

casperstorm avatar Apr 16 '25 16:04 casperstorm

Conventions say 80 is still preferred. Honestly I have no preference, but keeping things easily reviewable from mobile screens is nice and probably good to keep things at 80 for that reason.

tarkah avatar Apr 16 '25 16:04 tarkah

Ok, going with 80. I'll open another PR with the code file fixes...needs some time ;-)

/edit 1789 "violations", mostly line endings :-/

KaiKorla avatar Apr 16 '25 16:04 KaiKorla

@casperstorm @tarkah I removed Rust files from EditorConfig checks, because we're doing it with rusfmt. With #946 everything should be "green" then and we could integrate this RP.

KaiKorla avatar Apr 20 '25 15:04 KaiKorla

I think this could be useful, as evidenced by the documentation errors it has already caught. My only concern is whether there is any potential issue with having clippy run twice (by this action and the build action)? I think it could still be useful even if we don't have it run clippy.

Minor notes re: cspell.json:

  • I think vist should be left off (I think the CHANGELOG entry with vist is a typo of visit).
  • I don't see pkcs in the documentation so maybe it can be removed as well.

Looks like Megalinter is up to version 8.7.0 now :sweat_smile:

andymandias avatar May 28 '25 17:05 andymandias

This PR has been very fruitful in consolidating code style expectations and improving the existing GH actions. In particular, PRs #930, #1005, and #1008 owe a great deal to your contributions here. As a result of those PRs, however, the Megalinter checks are almost entirely redundant. Thank you for your help improving the contribution workflow, we greatly appreciate it, but I think we should close this PR in favor of the existing GH actions.

andymandias avatar May 31 '25 21:05 andymandias