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

Started to add rationales #5681

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

mildm8nnered
Copy link
Collaborator

@mildm8nnered mildm8nnered commented Jul 20, 2024

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

@SwiftLintBot
Copy link

SwiftLintBot commented Jul 20, 2024

2 Warnings
⚠️ If this is a user-facing change, please include a CHANGELOG entry to credit yourself!
You can find it at CHANGELOG.md.
⚠️ This PR may need tests.
17 Messages
📖 Linting Aerial with this PR took 0.93s vs 0.93s on main (0% slower)
📖 Linting Alamofire with this PR took 1.27s vs 1.27s on main (0% slower)
📖 Linting Brave with this PR took 7.24s vs 7.21s on main (0% slower)
📖 Linting DuckDuckGo with this PR took 5.14s vs 5.15s on main (0% faster)
📖 Linting Firefox with this PR took 10.76s vs 10.77s on main (0% faster)
📖 Linting Kickstarter with this PR took 10.01s vs 10.01s on main (0% slower)
📖 Linting Moya with this PR took 0.53s vs 0.53s on main (0% slower)
📖 Linting NetNewsWire with this PR took 2.92s vs 2.92s on main (0% slower)
📖 Linting Nimble with this PR took 0.77s vs 0.77s on main (0% slower)
📖 Linting PocketCasts with this PR took 8.68s vs 8.67s on main (0% slower)
📖 Linting Quick with this PR took 0.44s vs 0.44s on main (0% slower)
📖 Linting Realm with this PR took 4.52s vs 4.53s on main (0% faster)
📖 Linting Sourcery with this PR took 2.31s vs 2.32s on main (0% faster)
📖 Linting Swift with this PR took 4.55s vs 4.54s on main (0% slower)
📖 Linting VLC with this PR took 1.26s vs 1.27s on main (0% faster)
📖 Linting Wire with this PR took 17.94s vs 17.9s on main (0% slower)
📖 Linting WordPress with this PR took 11.43s vs 11.4s on main (0% slower)

Here's an example of your CHANGELOG entry:

* Started to add rationales.  
  [mildm8nnered](https://github.com/mildm8nnered)
  [#issue_number](https://github.com/realm/SwiftLint/issues/issue_number)

note: There are two invisible spaces after the entry's text.

Generated by 🚫 Danger

@mildm8nnered mildm8nnered force-pushed the mildm8nnered-add-rationale branch 5 times, most recently from 2e234b3 to e9e38be Compare July 29, 2024 22:08
@SimplyDanny
Copy link
Collaborator

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.

@mildm8nnered
Copy link
Collaborator Author

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.

@SimplyDanny
Copy link
Collaborator

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.

@mildm8nnered
Copy link
Collaborator Author

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 mildm8nnered force-pushed the mildm8nnered-add-rationale branch 2 times, most recently from 2db6532 to 7370528 Compare August 3, 2024 18:37
@SimplyDanny
Copy link
Collaborator

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.

👍

@mildm8nnered mildm8nnered force-pushed the mildm8nnered-add-rationale branch 5 times, most recently from c53b70e to 6ec46a0 Compare August 25, 2024 12:25
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-add-rationale branch 3 times, most recently from 132d531 to 1d9eaf6 Compare September 8, 2024 13:24
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-add-rationale branch 3 times, most recently from cfe760a to a5087ac Compare September 21, 2024 18:46
@mildm8nnered
Copy link
Collaborator Author

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

@SimplyDanny
Copy link
Collaborator

SimplyDanny commented Sep 21, 2024

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.

@mildm8nnered
Copy link
Collaborator Author

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 mildm8nnered force-pushed the mildm8nnered-add-rationale branch 2 times, most recently from 4f6079b to 3f4bbe9 Compare October 20, 2024 11:00
Copy link
Collaborator

@SimplyDanny SimplyDanny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also merge this piece by piece. Every new rational improves the rule. No need to wait for all rules being updated.

for variables and imports.

The `attributes_with_arguments_always_on_line_above`, `always_on_same_line`, and `always_on_line_above` \
configuration parameters can be used to fine-tune the rules behaviour for particular attributes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend not mentioning options in the rationale and rather plan to add dedicated rationals to the configuration elements.

rationale: """
See https://ericasadun.com/2016/10/02/quick-style-survey/ for discussion.

Summarizing here: "[Erica Sadun's] take on things after the poll and after talking directly with a number of \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could make use of quotes here like so:

Erica Sadun says:

> My name is Erica.

return line + "swift"
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this duplicated piece reusable somehow?

@mildm8nnered
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

@SimplyDanny
Copy link
Collaborator

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.

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.

3 participants