Skip to content

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

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