search

ionide / FsAutoComplete

F# language server using Language Server Protocol
Other
412 stars 154 forks source link

NegateBooleanExpression code fix #1252

Open nojaf opened 6 months ago

nojaf commented 6 months ago

NegateBooleanExpr

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

nojaf commented 6 months ago

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 commented 6 months ago

Either of those modes is acceptable to me

TheAngryByrd commented 6 months ago

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

nojaf commented 6 months ago

@TheAngryByrd which one do you prefer?

TheAngryByrd commented 6 months ago

stroustrup

nojaf commented 6 months ago

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 https://github.com/fsprojects/fantomas/issues/3070

baronfel commented 6 months ago

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 commented 6 months ago

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

nojaf commented 6 months ago

@brianrourkeboll would you mind taking a peek at https://github.com/ionide/FsAutoComplete/pull/1252/commits/90f17fa82e59a6ff42efd1b702bcfbf7fb66a913 I had to change some tests, so I'm probably doing something wrong.

brianrourkeboll commented 6 months ago

@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. https://github.com/dotnet/fsharp/pull/16973 isn't available yet, is it?

nojaf commented 6 months ago

The parens in those tests are indeed not needed. https://github.com/dotnet/fsharp/pull/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 commented 6 months ago

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 https://github.com/dotnet/fsharp/pull/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.