-
Notifications
You must be signed in to change notification settings - Fork 156
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
NegateBooleanExpression code fix #1252
base: nightly
Are you sure you want to change the base?
Conversation
can be avoided by not using Would you be on board to switch to |
Either of those modes is acceptable to me |
I’ve been wanting to move to either for a while just haven’t bit the bullet. Let’s do it. |
@TheAngryByrd which one do you prefer? |
stroustrup |
Yeah, this is something we want to do after the nightly ( |
Does this mean you want to wait a bit to merge this, or that we should merge this and change the formatting in a separate PR? |
I still need to improve this PR a bit regardless of formatting. I would do format configuration changes in a separate PR. |
18d87af
to
3db68a3
Compare
@brianrourkeboll would you mind taking a peek at 90f17fa |
The parens in those tests are indeed not needed. dotnet/fsharp#16973 isn't available yet, is it? |
No, it is not available in the nightlies. But I wasn't sure if that was the problem or if my code was just incorrect. I did not bother to use accurate ranges in the checks for the new code. I can't tell how relevant that part is. |
OK. Once that is available, I believe the API should no longer say parens are required for
I think that the ranges should not matter for the vast majority of hypothetical parenthesization scenarios (assuming dotnet/fsharp#16973 is in place). The times when they do matter are:
|
WIP, I'll need to circle back to this one when I encounter some more real-life situations.