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 F# Analyzers #603

Merged
merged 3 commits into from
Jul 25, 2024
Merged

Add F# Analyzers #603

merged 3 commits into from
Jul 25, 2024

Conversation

1eyewonder
Copy link
Contributor

Description

I thought it would be beneficial if we were to add some analyzers to the project in order to catch some areas we might improve. This opens up some discussion/thinking for the maintainers to consider configuration and additional pipeline opportunities.

  1. Do we want to add analyzers to the build pipeline/PR checks? See docs
  2. Are there specific analyzers we want to trigger errors/warning/ignore?

I don't have any strong opinions and just wanted to share some other cool opportunities we could try and add to this PR or others if desired.

How to test

  1. Run dotnet tool restore
  2. Run dotnet msbuild /t:AnalyzeSolution
  3. See all the suggestions
    image

Related issues

I was looking through some issues and came across #577 which made me think of suggesting this.

@64J0
Copy link
Member

64J0 commented Jul 15, 2024

Hello @1eyewonder, thanks for opening this PR. I'll review it during this week. For now, what I'd say is:

  1. Do we want to add analyzers to the build pipeline/PR checks?

This idea is interesting. It must be helpful for PR reviews. If you don't want to tackle it with this PR, we can do it later, no problem.

@1eyewonder
Copy link
Contributor Author

If we go ahead and tackle it within this PR, I am ok with it, but I think we'll have to coordinate. I believe the GitHub Advanced Security settings needed to be configured (see docs linked above).

A repo which I've seen this feature active on is Fable

Here are some additional links to provide visual/examples to the PR reviews since that seems to be the area of interest.

@64J0
Copy link
Member

64J0 commented Jul 17, 2024

Ack, thanks for the additional material @1eyewonder! I'll try to take a proper look at this PR either this or next week.

@64J0 64J0 force-pushed the fsharp-analyzers branch from 1ffe7f4 to 6f1e7d4 Compare July 21, 2024 21:19
@64J0 64J0 requested review from 64J0, dbrattli and nojaf July 21, 2024 21:19
@64J0 64J0 force-pushed the fsharp-analyzers branch from 6f1e7d4 to a1b2052 Compare July 25, 2024 00:45
@64J0
Copy link
Member

64J0 commented Jul 25, 2024

Cool, I think it's working:

image

Copy link
Member

@64J0 64J0 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@64J0 64J0 merged commit 5385767 into giraffe-fsharp:master Jul 25, 2024
9 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.

2 participants