Skip to content

Format all code - #1441

Merged
dsyme merged 4 commits into
fsprojects:mainfrom
nojaf:format
Jun 2, 2022
Merged

Format all code#1441
dsyme merged 4 commits into
fsprojects:mainfrom
nojaf:format

Conversation

@nojaf

@nojaf nojaf commented Jun 1, 2022

Copy link
Copy Markdown
Contributor

Hello, would you be interested in using Fantomas for this community project?

@nojaf

nojaf commented Jun 1, 2022

Copy link
Copy Markdown
Contributor Author

Fantomas is net6 only thing, is there any reason this project is still on net5?

@baronfel

baronfel commented Jun 1, 2022

Copy link
Copy Markdown
Contributor

even if there is a reason for sticking to net5.0 target frameworks, the project itself can and should move to 6.0 SDKs. You can target older TFMs from newer SDKs, you just also get the benefit of any fixes/feature enhancements that came with the newer SDK.

@nojaf

nojaf commented Jun 1, 2022

Copy link
Copy Markdown
Contributor Author

Right, so I should be able to bump the global json without many consequences right?

@baronfel

baronfel commented Jun 1, 2022

Copy link
Copy Markdown
Contributor

Yes, with a caveat - the PR/merge workflows use the setup-dotnet action to ensure the appropriate version of the .NET SDK is installed. If you change the value in global.json, I would also change these usages over to install multiple .NET SDKs to ensure that tests still run:

- name: Setup dotnet
  uses: actions/setup-dotnet@v2
  with:
    dotnet-version: | 
      5.0.x
      6.0.x

@nojaf

nojaf commented Jun 1, 2022

Copy link
Copy Markdown
Contributor Author

Right on! Thanks @baronfel.
@cartermp and @dsyme let me know what you prefer in this case.

@dsyme

dsyme commented Jun 2, 2022

Copy link
Copy Markdown
Contributor

Reformatting may be needed after pull

Regarding .NET version - we can update it - it might require an update to paket and fake?

maxNumberOfRows
=

using (logTime "LoadingTextToBeParsed" valueToBeParsedOrItsUri)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I will pre-change all explicit using to use _holder = ... to prevent this indentation happening. Then we reformat

@cartermp

cartermp commented Jun 2, 2022

Copy link
Copy Markdown
Collaborator

Updating to net6 is possible, it will just be annoying. IIRC this requires FAKE to move to be project-based

typeof<int>
typeof<int64> ]
typeof<float>,
[ typeof<Bit0>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please allow maximum list size large enough to allow these to be formatted single line

Comment thread src/Csv/CsvGenerator.fs Outdated
let ctor =
ProvidedConstructor(
[ for field in fields -> field.ProvidedParameter ],
invokeCode =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These invokeCode expand out a fair bit, I will pre-adjust them to be inner function bindings

Comment thread src/Xml/XmlRuntime.fs Outdated
text,
LoadOptions.PreserveWhitespace
)
.Root

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is strangely expanded. Is line width 80? I think it could be set to 120.

@dsyme

dsyme commented Jun 2, 2022

Copy link
Copy Markdown
Contributor

@nojaf Could you reformat after #1442 please?

@cartermp We should likely just cut FAKE out of this project

@dsyme

dsyme commented Jun 2, 2022

Copy link
Copy Markdown
Contributor

@nojaf I'm doing the update to .NET 6 separately here: #1443

@dsyme

dsyme commented Jun 2, 2022

Copy link
Copy Markdown
Contributor

@nojaf I'll do the merge and the things I mentioned above, and re-format

@dsyme

dsyme commented Jun 2, 2022

Copy link
Copy Markdown
Contributor

OK this is now ready - I'm happy with the code after formatting, more or less

@dsyme

dsyme commented Jun 2, 2022

Copy link
Copy Markdown
Contributor

I'll make a couple more tweaks to .edtiorconfig in a separate PR

@dsyme
dsyme merged commit 8639894 into fsprojects:main Jun 2, 2022
@dsyme

dsyme commented Jun 2, 2022

Copy link
Copy Markdown
Contributor

@nojaf Thank you sooooooo much for pushing this along. It makes the repository so much easier to maintain and contribute to

@nojaf

nojaf commented Jun 3, 2022

Copy link
Copy Markdown
Contributor Author

You are most welcome. The more projects are formatted, the more exposure for Fantomas 😸.

@nojaf
nojaf deleted the format branch June 3, 2022 07:46
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