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

NegateBooleanExpression code fix #1252

Draft
wants to merge 6 commits into
base: nightly
Choose a base branch
from

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Mar 24, 2024

NegateBooleanExpr

WIP, I'll need to circle back to this one when I encounter some more real-life situations.

@nojaf
Copy link
Contributor Author

nojaf commented Mar 28, 2024

Another nitpick here, but
image

can be avoided by not using cramped for https://fsprojects.github.io/fantomas/docs/end-users/Configuration.html#fsharp_multiline_bracket_style.

Would you be on board to switch to aligned or stroustrup for either these two files or all the code?

@baronfel @TheAngryByrd?

@baronfel
Copy link
Contributor

Either of those modes is acceptable to me

@TheAngryByrd
Copy link
Member

I’ve been wanting to move to either for a while just haven’t bit the bullet. Let’s do it.

@nojaf
Copy link
Contributor Author

nojaf commented Mar 29, 2024

@TheAngryByrd which one do you prefer?

@TheAngryByrd
Copy link
Member

stroustrup

@nojaf
Copy link
Contributor Author

nojaf commented Mar 30, 2024

Yeah, this is something we want to do after the nightly (8.0.300) is merged back into main.
Too many changes there. I also found fsprojects/fantomas#3070

@baronfel
Copy link
Contributor

Yeah, this is something we want to do after the nightly (8.0.300) is merged back into main. Too many changes there. I also found fsprojects/fantomas#3070

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?

@nojaf
Copy link
Contributor Author

nojaf commented Mar 31, 2024

I still need to improve this PR a bit regardless of formatting. I would do format configuration changes in a separate PR.

@nojaf nojaf force-pushed the NegateBooleanExpression branch from 18d87af to 3db68a3 Compare April 1, 2024 12:11
@nojaf
Copy link
Contributor Author

nojaf commented Apr 3, 2024

@brianrourkeboll would you mind taking a peek at 90f17fa
I had to change some tests, so I'm probably doing something wrong.

@brianrourkeboll
Copy link
Contributor

@brianrourkeboll would you mind taking a peek at 90f17fa I had to change some tests, so I'm probably doing something wrong.

The parens in those tests are indeed not needed. dotnet/fsharp#16973 isn't available yet, is it?

@nojaf
Copy link
Contributor Author

nojaf commented Apr 3, 2024

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.

@brianrourkeboll
Copy link
Contributor

brianrourkeboll commented Apr 3, 2024

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.

OK. Once that is available, I believe the API should no longer say parens are required for not (a), etc.

I did not bother to use accurate ranges in the checks for the new code. I can't tell how relevant that part is.

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:

  1. Determining the presence of "sensitive indentation," which shouldn't exist for code that isn't already parenthesized, since otherwise it would already fail to build — although I guess there could be some scenarios involving multiline expressions where you'd need to pay attention to where the new offsides line would be, depending on what exactly you were doing.
  2. Determining whether certain constructs precede certain other constructs on the same line because of odd parsing behavior, e.g., nested if/then/elses on the same line, nested match-like constructs on the same line, typed expressions on the same line before a match-clause arrow, etc. This likewise shouldn't be a problem for most code fix scenarios, since again problematic code would already be causing a compiler error.

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.

4 participants