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

Add FavourAsKeyword rule #709

Merged
merged 6 commits into from
Jun 9, 2024

Conversation

ribeirotomas1904
Copy link
Contributor

Add FavourAsKeyword rule and tests and docs for it.
Fixes #664

@knocte
Copy link
Collaborator

knocte commented Jun 8, 2024

Thanks for your contribution!

I'm going to do a quick review, but by the looks of it, most things are just going to be nitpicks; except one: please don't forget do add the new rule config entry in fsharplint.json: https://github.com/fsprojects/FSharpLint/blob/master/src/FSharpLint.Core/fsharplint.json

"""

Assert.AreEqual(expected, this.ApplyQuickFix source)
this.AssertErrorWithMessageExists("Prefer using the as pattern to match a constant and bind it to a variable.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ribeirotomas1904 let's rather have two tests: one for the AssertErrorWithMessageExists and another one for the QuickFix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 573f0e0

</data>
</data>
<data name="RulesFavourAsKeyword" xml:space="preserve">
<value>Prefer using the as pattern to match a constant and bind it to a variable.</value>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ribeirotomas1904 nit, let's wrap the word "as" with single-quotes, to make this more readable (otherwise at first glance it's hard to see that as is meant to be read as a keyword)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in c985a31

@ribeirotomas1904
Copy link
Contributor Author

Thanks for your contribution!

I'm going to do a quick review, but by the looks of it, most things are just going to be nitpicks; except one: please don't forget do add the new rule config entry in fsharplint.json: https://github.com/fsprojects/FSharpLint/blob/master/src/FSharpLint.Core/fsharplint.json

Thanks for the feedback!

config entry for the new rule added in 2f80c7e

@knocte knocte merged commit 873d145 into fsprojects:master Jun 9, 2024
5 checks passed
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.

FavourAsKeyword
2 participants