SwiftLint icon indicating copy to clipboard operation
SwiftLint copied to clipboard

Started to add rationales

Open mildm8nnered opened this issue 1 year ago • 10 comments

Addresses #4811

The idea is to add a longer explanation of the purpose or motivation behind each rule.

You can now see the output via swiftlint rules rule_name

For example

% swiftlint.debug rules array_init
Array Init (array_init): Prefer using `Array(seq)` over `seq.map { $0 }` to convert a sequence into an Array

Rationale:

When converting the elements of sequence directly into an `Array`, for clarity, prefer using the `Array` constructor over calling `map`. For example

    Array(foo)

rather than

    foo.↓map({ $0 })

If some processing of the elements is required, then using `map` is fine. For example

    foo.map { !$0 }

Constructs like

    enum MyError: Error {}
    let myResult: Result<String, MyError> = .success("")
    let result: Result<Any, MyError> = myResult.map { $0 }

may be picked up as false positives by the `array_init` rule. If your codebase contains constructs like this, consider using the `typesafe_array_init` analyzer rule instead.

And when rendered via Jazzy:

Screenshot 2024-07-30 at 00 38 03

mildm8nnered avatar Jul 20 '24 13:07 mildm8nnered

1 Warning
:warning: This PR may need tests.
18 Messages
:book: Building this branch resulted in a binary size of 29581.29 KiB vs 29580.32 KiB when built on main (0% larger).
:book: Linting Aerial with this PR took 1.67 s vs 0.94 s on main (77% slower).
:book: Linting Alamofire with this PR took 1.28 s vs 1.28 s on main (0% slower).
:book: Linting Brave with this PR took 8.65 s vs 8.62 s on main (0% slower).
:book: Linting DuckDuckGo with this PR took 5.82 s vs 5.84 s on main (0% faster).
:book: Linting Firefox with this PR took 11.88 s vs 11.91 s on main (0% faster).
:book: Linting Kickstarter with this PR took 9.98 s vs 9.99 s on main (0% faster).
:book: Linting Moya with this PR took 0.55 s vs 0.53 s on main (3% slower).
:book: Linting NetNewsWire with this PR took 2.9 s vs 2.9 s on main (0% slower).
:book: Linting Nimble with this PR took 0.79 s vs 0.79 s on main (0% slower).
:book: Linting PocketCasts with this PR took 8.7 s vs 8.69 s on main (0% slower).
:book: Linting Quick with this PR took 0.44 s vs 0.44 s on main (0% slower).
:book: Linting Realm with this PR took 4.62 s vs 4.57 s on main (1% slower).
:book: Linting Sourcery with this PR took 2.35 s vs 2.36 s on main (0% faster).
:book: Linting Swift with this PR took 4.82 s vs 4.81 s on main (0% slower).
:book: Linting VLC with this PR took 1.33 s vs 1.33 s on main (0% slower).
:book: Linting Wire with this PR took 18.79 s vs 18.84 s on main (0% faster).
:book: Linting WordPress with this PR took 11.5 s vs 11.51 s on main (0% faster).

Generated by :no_entry_sign: Danger

SwiftLintBot avatar Jul 20 '24 13:07 SwiftLintBot

That's an ambitious plan with 238 existing rules. 😅 But I very much appreciate the effort. This definitely something helpful and can be extended over time with the infrastructure as a starting point.

A few thoughts from my side:

  • There are currently one description and sometimes additional more specific reasons per rule. Now, we would add another form of documentation with the rational. I wonder if we should rather extend the description and introduce something like a "default reason" or "violation reason".
  • Something similar has been on my ToDo list for rule configuration options for quite a while.
  • I think that the text should be entirely Markdown.

SimplyDanny avatar Jul 30 '24 19:07 SimplyDanny

That's an ambitious plan with 238 existing rules. 😅 But I very much appreciate the effort. This definitely something helpful and can be extended over time with the infrastructure as a starting point.

238 is a lot yes, but I think definitely achievable. I've had a lot of fun writing the ones so far, and it's the kind of thing that you can knock some off on a rainy afternoon. Keep going, and before you know it you're done.

I think one think to talk about is "voice" - we could go for something very dry and technical, or somewhat more lighthearted, or even something a bit zany.

I think my take is that some kind of middle ground is good, but it would be nice to have some "character" to the rationale's, and to let them be a little bit opionated in places ("almost no-one enables this rule").

A few thoughts from my side:

  • There are currently one description and sometimes additional more specific reasons per rule. Now, we would add another form of documentation with the rational. I wonder if we should rather extend the description and introduce something like a "default reason" or "violation reason".

So I think description has a role, and it's short and widely used.

I kind of see rationale as more of a freeform piece of text, that might mention some original source of why X is bad/good, and/or also maybe give some tips about using the rule, or about configuration options that you might want to use, or that people commonly set.

"Why should I care about this rule/how should I get the most out of it", together with a bit more of a narrative explanation of what it actually is - sometimes the examples can be a little obtuse.

  • Something similar has been on my ToDo list for rule configuration options for quite a while.

Yep - I think it would be great to have something more formal for config options.

It's kind of a pain, because we probably have a lot, and someone has to write the text (see rationale's), but we could definitely do a better job of explaining what individual options do.

I think mentions of config options in rationale's are of the kind - "this option is one that you might want to use" rather than documenting the config options per se.

  • I think that the text should be entirely Markdown.

My kind of tacky solution was

  1. needs to look good in swiftlint rules rule_name output (would be so nice if that picked up the system pager, if set)
  2. needs to look good in Jazzy output

So strip "```" for the Terminal and add swift for Jazzy, and make sure you get your indentation right for the Terminal yourself.

I think the Terminal case is harder.

Adding swift to "```" I kind of saw a service to rationale authors - it's probably swift code, and I know we'll forget to add those manually.

mildm8nnered avatar Jul 30 '24 20:07 mildm8nnered

That's an ambitious plan with 238 existing rules. 😅 But I very much appreciate the effort. This definitely something helpful and can be extended over time with the infrastructure as a starting point.

238 is a lot yes, but I think definitely achievable. I've had a lot of fun writing the ones so far, and it's the kind of thing that you can knock some off on a rainy afternoon. Keep going, and before you know it you're done.

Oh, perfect. Happy to learn you like writing documentation. 😉

I think one think to talk about is "voice" - we could go for something very dry and technical, or somewhat more lighthearted, or even something a bit zany.

I think my take is that some kind of middle ground is good, but it would be nice to have some "character" to the rationale's, and to let them be a little bit opionated in places ("almost no-one enables this rule").

I think all these styles are fine. And maybe even a mixture makes them interesting for readers. One consistent style throughout the whole set of rules is anyway almost impossible.

  • I think that the text should be entirely Markdown.

My kind of tacky solution was

  1. needs to look good in swiftlint rules rule_name output (would be so nice if that picked up the system pager, if set)
  2. needs to look good in Jazzy output

So strip "```" for the Terminal and add swift for Jazzy, and make sure you get your indentation right for the Terminal yourself.

I think the Terminal case is harder.

The thing is that without using Markdown, there is no good way to use different text styles and add links to other rules or further references. For the terminal output, all the styling modifiers could be removed afterwards.

Adding swift to "```" I kind of saw a service to rationale authors - it's probably swift code, and I know we'll forget to add those manually.

Important is that it allows to choose other languages as well.

SimplyDanny avatar Jul 31 '24 21:07 SimplyDanny

That's an ambitious plan with 238 existing rules. 😅 But I very much appreciate the effort. This definitely something helpful and can be extended over time with the infrastructure as a starting point.

238 is a lot yes, but I think definitely achievable. I've had a lot of fun writing the ones so far, and it's the kind of thing that you can knock some off on a rainy afternoon. Keep going, and before you know it you're done.

Oh, perfect. Happy to learn you like writing documentation. 😉

:-)

I think one think to talk about is "voice" - we could go for something very dry and technical, or somewhat more lighthearted, or even something a bit zany. I think my take is that some kind of middle ground is good, but it would be nice to have some "character" to the rationale's, and to let them be a little bit opionated in places ("almost no-one enables this rule").

I think all these styles are fine. And maybe even a mixture makes them interesting for readers. One consistent style throughout the whole set of rules is anyway almost impossible.

I hadn't thought of it like that, but actually I think that is absolutely the best approach. Nothing too prescriptive, and then we can catch anything that goes too far off-piste, or lacks clarity in individual CR.

I think comprehensibility to English as a second language speakers is important, so probably we wouldn't want rationale's in Yorkshire Dialect or Runyonese, sadly.

  • I think that the text should be entirely Markdown.

My kind of tacky solution was

  1. needs to look good in swiftlint rules rule_name output (would be so nice if that picked up the system pager, if set)
  2. needs to look good in Jazzy output

So strip "```" for the Terminal and add swift for Jazzy, and make sure you get your indentation right for the Terminal yourself. I think the Terminal case is harder.

The thing is that without using Markdown, there is no good way to use different text styles and add links to other rules or further references. For the terminal output, all the styling modifiers could be removed afterwards.

Any tips for how you think we should go about the styling modifiers stripping? Not entirely keen on having a markdown parser just so that I can de-markdown it :-)

Adding swift to "```" I kind of saw a service to rationale authors - it's probably swift code, and I know we'll forget to add those manually.

Important is that it allows to choose other languages as well.

So the intention is to ignore any language code that's present, as an override.

mildm8nnered avatar Aug 01 '24 20:08 mildm8nnered

I hadn't thought of it like that, but actually I think that is absolutely the best approach. Nothing too prescriptive, and then we can catch anything that goes too far off-piste, or lacks clarity in individual CR.

I think comprehensibility to English as a second language speakers is important, so probably we wouldn't want rationale's in Yorkshire Dialect or Runyonese, sadly.

Fully support that, especially the last part! 🫣

The thing is that without using Markdown, there is no good way to use different text styles and add links to other rules or further references. For the terminal output, all the styling modifiers could be removed afterwards.

Any tips for how you think we should go about the styling modifiers stripping? Not entirely keen on having a markdown parser just so that I can de-markdown it :-)

Well, there is swiftlang/swift-markdown from Apple which I had been playing around with a little at that time. A new dependency should be well thought of. As these rationales are going to provide such a huge benefit, that would be justified though I guess. Apart from removing any Markdown styling, it would also allow SwiftLint to check that the whole text is actually valid Markdown before rendering the docs in the build.

On the other hand, even unrendered Markdown is so well readable, that I wouldn't complain if we just print the text as is into the console (keeping also URLs). There are also console rendering tools available that could be used to prettify the output. That, however, oughtn't be SwiftLint's job then.

Adding swift to "```" I kind of saw a service to rationale authors - it's probably swift code, and I know we'll forget to add those manually.

Important is that it allows to choose other languages as well.

So the intention is to ignore any language code that's present, as an override.

👍

SimplyDanny avatar Aug 11 '24 11:08 SimplyDanny

Are these CI failures something that I'm doing @SimplyDanny ? - https://buildkite.com/swiftlint/swiftlint/builds/8753 - they all seem to be concurrency related, but it feels like they come and go at random

mildm8nnered avatar Sep 21 '24 19:09 mildm8nnered

Are these CI failures something that I'm doing @SimplyDanny ? - https://buildkite.com/swiftlint/swiftlint/builds/8753 - they all seem to be concurrency related, but it feels like they come and go at random

They fail randomly. It's annoying. A retrigger normally helps. I hope this gets fixed by having strict concurrency checking always on by default. But until then there's still a long way to go.

Optionally, we can disable warnings reported as errors in Bazel builds for the time being.

SimplyDanny avatar Sep 21 '24 19:09 SimplyDanny

Are these CI failures something that I'm doing @SimplyDanny ? - https://buildkite.com/swiftlint/swiftlint/builds/8753 - they all seem to be concurrency related, but it feels like they come and go at random

They fail randomly. It's annoying. A retrigger normally helps. I hope this gets fixed by having strict concurrency checking always on by default. But until then there's still a long way to go.

Optionally, we can disable warnings reported as errors in Bazel builds for the time being.

It feel like mine always fail :-) Apart from pushing an empty commit, is there a better way to retrigger them?

mildm8nnered avatar Sep 22 '24 00:09 mildm8nnered

Are these CI failures something that I'm doing @SimplyDanny ? - https://buildkite.com/swiftlint/swiftlint/builds/8753 - they all seem to be concurrency related, but it feels like they come and go at random

They fail randomly. It's annoying. A retrigger normally helps. I hope this gets fixed by having strict concurrency checking always on by default. But until then there's still a long way to go. Optionally, we can disable warnings reported as errors in Bazel builds for the time being.

It feel like mine always fail :-) Apart from pushing an empty commit, is there a better way to retrigger them?

This should be much more stable now. I haven't seen the random failures since the update to Swift 6 and a few code adaptions.

SimplyDanny avatar Oct 07 '24 20:10 SimplyDanny

I'm going to pick this up again soon.

I just tried Copilot for the first time in a while ... quite interesting

Screenshot 2024-11-04 at 21 25 40
Screenshot 2024-11-04 at 21 26 06

mildm8nnered avatar Nov 04 '24 21:11 mildm8nnered

Screenshot 2024-11-05 at 17 52 14

This one took a bit more prompting - initially it just talked in generic terms about modern hash methods.

mildm8nnered avatar Nov 05 '24 17:11 mildm8nnered

Using the Copilot to craft rule descriptions is a very good idea. This can reduce the effort a lot. 👍

For coding in SwiftLint it's often not very helpful unfortunately.

SimplyDanny avatar Nov 17 '24 10:11 SimplyDanny

@mildm8nnered: All my remarks have been addressed. You are free to merge this once it's ready from your point of view. 👍

SimplyDanny avatar Feb 17 '25 09:02 SimplyDanny