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

FS-1146-scoped-nowarn RFC proposal #782

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Martin521
Copy link
Contributor

@Martin521 Martin521 commented Aug 7, 2024

RFC proposal for scoped nowarn.
Click here to view the latest version.
For discussions, #786 has been started.

@Martin521 Martin521 marked this pull request as draft August 7, 2024 21:12
@Martin521 Martin521 mentioned this pull request Aug 9, 2024
22 tasks
8. `--nowarn` compiler flags are not considered for processing `#warnon` directives. This means that for a warning number `n` that is disabled by a compiler flag

a) `#warnon` for `n` without previous `#nowarn` for `n` is an error.

Copy link
Contributor

@T-Gro T-Gro Aug 11, 2024

Choose a reason for hiding this comment

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

I understand the perspective of protecting against user mistakes, by calling it an error.
But I do not like the UX for the following scenario:

  • A huge project, and I want to ignore a warning in just 1 file out of many.

Would the recommendation be to use:

  • --nowarn 123 on project level
  • #nowarn 123 in top of the file
  • immediately followed by #warnon 123 right below it ?

I am wondering if the repetitive #nowarn would be needed.
The F# compilation could be changed to allow the following pairs:

  1. Valid pairs of #nowarn and #warnon
  2. A pair of project level --nowarn and a file level #warnon
    And produce an error in remaining cases

Copy link
Contributor Author

@Martin521 Martin521 Aug 11, 2024

Choose a reason for hiding this comment

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

@T-Gro Thanks for the feedback. This RFC is still in an early phase and therefore marked draft.
My plan was to get a bit further in understanding the implications of these items on the implementation and then open a discussion thread for this RFC.
If it is ok for you I would wait with a response until then.

Edit: I think the current proposal has dealt with that concern now.

@Martin521 Martin521 changed the title FS-1146-scoped-nowarn initial RFC doc FS-1146-scoped-nowarn RFC proposal Aug 19, 2024
@Martin521
Copy link
Contributor Author

Martin521 commented Aug 19, 2024

So, here is a full proposal.
Feedback is appreciated. Either in the discussion #786, or per review.

@Martin521 Martin521 marked this pull request as ready for review August 19, 2024 15:18
@T-Gro
Copy link
Contributor

T-Gro commented Sep 13, 2024

Heads up:

The following section will need a rewrite showing the logic and a few samples (I assume those can be taken from tests from the implementation PR once it lands):

https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/compiler-directives#preprocessor-directives

@Martin521
Copy link
Contributor Author

Can this PR be merged?
It is still a draft then and can still be changed, but at least we have a starting point.

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