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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fantomas update #15847

Merged
merged 2 commits into from
Nov 22, 2023
Merged

Fantomas update #15847

merged 2 commits into from
Nov 22, 2023

Conversation

dawedawe
Copy link
Contributor

This updates the Fantomas tool from 5.0.3 to 6.1.2
As a result, the codebase is much more in line with the community style guide.

Yes, there will be some pain but it's one of these the-longer-we-wait-the-worse-it-gets things.
One could argue, we already waited too long.
Most of the changes are about single space changes, so I'd say it's not all that bad.

A lot of bugs were fixed since 5.0.3, so we could remove some files from .fantomasignore.
But to keep this PR as small as possible, this is not yet done.

A big win will be the parallel processing we have since 6.0, thanks to @TheAngryByrd
Locally I'm seeing a speedup > 2x: dotnet fantomas . --check going down from ~15s to ~7s.

Best of all, we now have this enjoyable colourful output 馃槈
image

If you're interested in all the changes:
Here's our upgrage guide
Here are our releases

@dawedawe dawedawe requested a review from a team as a code owner August 23, 2023 15:07
@dawedawe
Copy link
Contributor Author

dawedawe commented Aug 23, 2023

Mmh, does someone know why Azure DevOps can't find 6.1.2?

/tmp/3f16bc83-a26f-44a3-9aa0-39087465e983/restore.csproj : error NU1102: Unable to find package fantomas with version (= 6.1.2)
/tmp/3f16bc83-a26f-44a3-9aa0-39087465e983/restore.csproj : error NU1102:   - Found 209 version(s) in dotnet-public [ Nearest version: 5.0.3 ]

Edit:
Seems like we need to ask ".NET Core Eng Service Partners" --> "First Responders" to add the package version to the feed. Is this some kind of internal MS team?

@vzarytovskii
Copy link
Member

I would like to ask you to postpone fantomas upgrade until we merge big nullness PR, otherwise it will be a merge nightmare.

@dawedawe
Copy link
Contributor Author

I would like to ask you to postpone fantomas upgrade until we merge big nullness PR, otherwise it will be a merge nightmare.

Yes, sure.

@psfinaki
Copy link
Member

Yes, there will be some pain but it's one of these the-longer-we-wait-the-worse-it-gets things.
One could argue, we already waited too long.

No, it's not too long. Thanks for this. We'll do that :)
Maybe it would be useful to keep the branch in sync though.

@dawedawe
Copy link
Contributor Author

No, it's not too long. Thanks for this. We'll do that :) Maybe it would be useful to keep the branch in sync though.

Nice to read that, thanks. I'll try to keep it in sync.
If it falls out-of-sync and the team thinks the time has come to do this update, just ping me.

azure-pipelines.yml Outdated Show resolved Hide resolved
@nojaf
Copy link
Contributor

nojaf commented Nov 22, 2023

I would like to ask you to postpone fantomas upgrade until we merge big nullness PR, otherwise it will be a merge nightmare.

As some time has passed now, is there any update on this @vzarytovskii?
Is this nullness PR still in the works?

@vzarytovskii
Copy link
Member

I would like to ask you to postpone fantomas upgrade until we merge big nullness PR, otherwise it will be a merge nightmare.

As some time has passed now, is there any update on this @vzarytovskii?

Is this nullness PR still in the works?

It is. And will be for some time. Any non-functional (stylistic) change will make it much harder for merging.

Fantomas can be updated and applied at any point and not a necessary update. So, I'd still like to ask to postpone the update.

@nojaf
Copy link
Contributor

nojaf commented Nov 22, 2023

Any non-functional (stylistic) change will make it much harder for merging.

I don't know if I really buy that, to be honest.
You sound rather theoretical, versus speaking from actual experience with this codebase and upgrading this tool.

And if the PR formats the code with the same version as on the main I would expect untouched code to remain untouched afterwards.

@vzarytovskii
Copy link
Member

Alright, gonna update it if you insist.

@T-Gro I will take care of merging main to nullables pr, but might need some help with metadata emitting part if I will get stuck

@nojaf
Copy link
Contributor

nojaf commented Nov 22, 2023

Alright, gonna update it if you insist.

Thanks, I merely want to challenge you a little bit about the waiting part. Give it a shot, if you are ten minutes in and it appears a sh*tshow then just don't do it. But then at least we know, and that settles it.

@vzarytovskii vzarytovskii enabled auto-merge (squash) November 22, 2023 15:08
@vzarytovskii vzarytovskii merged commit 5cd52b7 into dotnet:main Nov 22, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants