Clean-up project files and MSBuild logic #3893

Nirmal4G · 2021-03-26T18:51:22Z

Fixes #3896

Fix corrupted emojis and symbols
Fix-up white-spaces and new-lines
Remove un-necessary and duplicate logic
Rename missed files from previous renames
Fix Markdown tests with trailing white-spaces

PR Type

What kind of change does this PR introduce?

Code style update (formatting)
Refactoring (non-breaking functional changes but no API changes)

What is the current behavior?

It's hard to read and understand the repo because of the redundant logic and un-necessary new-lines and white-spaces.

What is the new behavior?

It's now little bit better to read and understand the repo now that we cleaned it up.

PR Checklist

Please check if your PR fulfils the following requirements:

Tested code with current supported SDKs
Header has been added to all new source files (run build/UpdateHeaders.bat)
Contains NO breaking changes

Other information

if you're editing this patch tree, please rebase on latest HEAD and then commit, without updating from the latest HEAD.
For merging this patch, please use fast-forward or rebase option if possible.
That way, individual commits are preserved since, each commit has large set of changes.

ghost · 2021-03-26T18:51:24Z

Thanks Nirmal4G for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

michael-hawker · 2021-03-29T17:11:42Z

Appreciate the initiative here @Nirmal4G. Can we start with just a whitespace clean-up with no other changes to logic/structure though? It's hard to review something touching so many files when most of it is just whitespace. Then we can start other PRs with other proposed changes.

There's also a git setting we can enable for this to help maintain consistency moving forward; as this was called out on a docs PR to do a similar task as well.

Was there a tool you used to automate this task as well?

Nirmal4G · 2021-03-29T19:06:28Z

Was there a tool you used to automate this task as well?

Yes. For removing BOM, I used unix2dos CLI tool. For removing trailing white-space(s) and duplicate new-line(s), I used VS IDE Clean-up. Rest all are 🙌 hand-made!!

Rosuavio

I think we should keep the GhostDoc* files as we don't have any other bespoke spell/grammar checker, unlike the *DotSettings files which are for ReSharper and we already document a recommended IDK (VS 2017 and up).

Nirmal4G · 2021-03-31T17:51:39Z

For only spelling check, you have one of the best in the VS Extensions Store: VS Spell Checker
Besides, I only removed those files since the last modified is almost 2 years ago. If it was actively used, there would be recent changes to those files.
Also, the name hasn't changed since the last solution rename. I doubt it would even load into VS IDE, for those who actually have the extension.

ghost · 2021-03-31T17:56:58Z

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

Rosuavio

Some more minor changes.

Microsoft.Toolkit.HighPerformance/Buffers/StringPool.cs

Microsoft.Toolkit.HighPerformance/Memory/Memory2D{T}.cs

Microsoft.Toolkit.HighPerformance/Memory/ReadOnlyMemory2D{T}.cs

Microsoft.Toolkit.Uwp.UI.Controls.DataGrid/CollectionViews/ListCollectionView.cs

Microsoft.Toolkit.Uwp.UI.Controls.DataGrid/DataGrid/DataGridColumnHeader.cs

Microsoft.Toolkit.Uwp.UI/Extensions/FrameworkElement/FrameworkElementExtensions.LogicalTree.cs

Nirmal4G · 2021-03-31T18:26:58Z

These changes are done by dotnet-format. Guess it's not reliable, after all! I'll check and revert those changes. Thanks for looking through.

Rosuavio · 2021-03-31T19:15:11Z

No problem @Nirmal4G, I think I will raise issues on https://github.com/dotnet/format and see what they say. It could be that we have some particular edge cases that they did not test/account for.

Nirmal4G · 2021-05-18T18:51:45Z

@RosarioPulella I did check. It doesn't work! So, I reverted it back to text file type. I have also formatted the file little bit too!

@michael-hawker @Sergio0694 All done for this PR. If anything remains, I'll do it in #3894. Is that okay?

michael-hawker

@Nirmal4G it'd be good if we could figure out how to run the tools you're using as part of our check-in process or as a validator or something from our cake script to ensure new changes are consistent with newlines and XML formatting.

Is this something you think you could help us with too as part of this PR?

michael-hawker · 2021-05-20T20:23:14Z

Microsoft.Toolkit.Diagnostics/Microsoft.Toolkit.Diagnostics.csproj

-
-        <!-- .NET Standard 1.4 doesn't have the Span<T> type, ValueTuple or the [Pure] attribute -->


I'm either way on the extra blank line, but especially with the ones below, the comments are about the packages, so it seems a bit odd to move them to outside the item group?

Nirmal4G · 2021-05-20T21:06:04Z

@michael-hawker The formatting I did in this PR is handmade!

I'm also looking into updating the Cake build with format validation. But the codebase also contains some files that are custom formatted. Those won't be preserved, if I put any automated formatting tool into the build process. There are a few bug fixes coming after the PRs that I already have. Let me finish those, and then, I'll look into this. Is that okay?

michael-hawker · 2021-06-01T18:13:12Z

Thanks @Nirmal4G, sorry for the delay, //Build ate up some time.

It's helpful for us to have focus on a PR to solve a specific problem vs. multiple things at once. This is something we should have in our guidelines for the project in the wiki, and something we plan to help outline more in the near future.

On that note, this PR is still pretty huge and trying to do many different things. We should really separate it again:

1 & 2 & 5 for all the whitespace/emoji fixes seem fine to have together
4 for the file rename would be easier to review on its own quickly

For part 3, cleaning up the MSBuild logic is fine to remove potential issues; but the rest of just re-ordering things or moving comments is causing a lot of churn. If there's an automated way to do something great. We can add something to our build scripts and then run that once against the repo to organize stuff. Otherwise it's not maintainable as if we fix it now it'll get broken in the future.

We're also currently trying to maintain two branches with everything for WinUI 3, so the last PR for BOM/whitespace caused a ton of manual merge conflicts that needed to be resolved.

Love your enthusiasm here in this space, but we just need to be cognizant of how we can maintain things across the project as a whole now and in the future.

If you could split this up and apply this general feedback, then it'll be a lot easier for us to review and merge in changes to unblock the rest of your PRs. Thanks!

Nirmal4G · 2021-06-01T19:10:43Z

@michael-hawker I'm so sorry for the big PRs. But maintenance PRs are expected to be huge. This patch tree is final, since, the changes here completes the next PR. And that'll be the last of the refactorings.

I've sent additional replies through Discord.

As we save files with trailing white-space removed, the markdown parser tests that depends on the text with trailing white-space fails. So, to prevent any future formatting changes affecting the test result, we'll replace the actual trailing space(s) with an interpolated variable containing those trailing space(s).

The emojis and symbols applied here were corrupted since the following patch. ``` Commit: 7da4442 Author: David Catuhe <david.catuhe@live.fr> Date : Fri Jul 15 07:51:16 2016 -0700 Title : Fixed encodings ```

Runtime Directive files and the text files that have been missed in previous renames have been renamed as proposed!

Add/Remove new-lines (only in project files, xml files and some source files)

Add/Remove leading and trailing white-space (only in comments, code blocks, xml strings)

Format comments to be legible. - Place comment start and end tags on a new line for multi-line comments. - Have space between start and end tags in a single line comment. Place comments where appropriate. - If an entire block is common to the comment then place it above the block. - Only place a comment near or after the block, if it refers exclusively. Sources are not formatted!

To make diff understandable through blame and across similar project files, I have re-organized some lines in these project files. This also improves readability. Previously, the focus is per-project file but now—to maintain project logic across repos and several similar logic across project files, new way of authoring project files is required. This refactor is the start of it.

Change Nuget -> NuGet

Remove commented build logic Remove duplicated build logic

Override projects with an empty Pack target that are not packed into packages. This will prevent any any packages being accidentally produced.

These files were not formatted by the dotnet format tool. So, manually formatting it by hand!

The .NET Formatter breaks in these scenarios. Improves maintainability of code in the long run.

Update renamed files in the build logic Use correct path format in the build logic

ghost requested review from michael-hawker, azchohfi, Kyaa-dost and Rosuavio March 26, 2021 18:51

Nirmal4G changed the title ~~clean-up repo and project files~~ Cleanup repo and project files Mar 26, 2021

Nirmal4G force-pushed the hotfix/clean-up branch 3 times, most recently from 1825545 to 37819af Compare March 27, 2021 00:13

Nirmal4G changed the title ~~Cleanup repo and project files~~ Clean-up repo and project files Mar 27, 2021

Nirmal4G changed the title ~~Clean-up repo and project files~~ Clean-up project files and MSBuild logic Mar 27, 2021

Nirmal4G marked this pull request as ready for review March 27, 2021 01:56

Kyaa-dost added the improvements ✨ label Mar 29, 2021

Nirmal4G force-pushed the hotfix/clean-up branch 5 times, most recently from b98897c to fa6f017 Compare March 31, 2021 14:33

Rosuavio suggested changes Mar 31, 2021

View reviewed changes

ghost added the needs author feedback 📝 label Mar 31, 2021

ghost added needs attention 👋 and removed needs author feedback 📝 labels Mar 31, 2021

Rosuavio suggested changes Mar 31, 2021

View reviewed changes

ghost added the needs author feedback 📝 label Mar 31, 2021

Nirmal4G mentioned this pull request May 19, 2021

'System.Text.Json.ReflectionMemberAccessor' could not be found #4032

Open

michael-hawker reviewed May 20, 2021

View reviewed changes

michael-hawker mentioned this pull request May 20, 2021

Update package titles and consolidate build logic #3894

Merged

4 tasks

ghost removed the needs attention 👋 label Jun 1, 2021

Nirmal4G force-pushed the hotfix/clean-up branch from 8d31aab to 1071f50 Compare June 2, 2021 17:56

Nirmal4G requested a review from michael-hawker June 2, 2021 17:58

Nirmal4G added 2 commits June 4, 2021 09:00

Fix corrupted emojis and symbols

e606ae8

The emojis and symbols applied here were corrupted since the following patch. ``` Commit: 7da4442 Author: David Catuhe <david.catuhe@live.fr> Date : Fri Jul 15 07:51:16 2016 -0700 Title : Fixed encodings ```

Nirmal4G force-pushed the hotfix/clean-up branch from 1071f50 to c0fc313 Compare June 4, 2021 08:27

Nirmal4G added 11 commits June 4, 2021 21:00

Rename missed files from previous renames

c6d9fe4

Runtime Directive files and the text files that have been missed in previous renames have been renamed as proposed!

Fix-up new-lines across project files

06362b2

Add/Remove new-lines (only in project files, xml files and some source files)

Fix-up white-spaces across project files

7e37935

Add/Remove leading and trailing white-space (only in comments, code blocks, xml strings)

Use correct casing for certain terminologies

53a7635

Change Nuget -> NuGet

Remove un-necessary build logic across project files

54ffaf9

Remove commented build logic Remove duplicated build logic

Remove projects that are not required to be packed

7c61d09

Override projects with an empty Pack target that are not packed into packages. This will prevent any any packages being accidentally produced.

Format missed files manually

aa5f174

These files were not formatted by the dotnet format tool. So, manually formatting it by hand!

Use Type aliasing instead of macros

a52ad35

The .NET Formatter breaks in these scenarios. Improves maintainability of code in the long run.

Consolidate package versions

c891e29

Update renamed files in the build logic Use correct path format in the build logic

Nirmal4G force-pushed the hotfix/clean-up branch from c0fc313 to c891e29 Compare June 5, 2021 13:12

michael-hawker approved these changes Jun 15, 2021

View reviewed changes

michael-hawker merged commit 6c63d55 into CommunityToolkit:main Jun 15, 2021

Nirmal4G deleted the hotfix/clean-up branch June 16, 2021 02:56

Sergio0694 mentioned this pull request Jun 23, 2021

Fix register/unregister issues of WeakReferenceMessenger #4082

Merged

8 tasks

michael-hawker added this to the 7.1 milestone Aug 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean-up project files and MSBuild logic #3893

Clean-up project files and MSBuild logic #3893

Nirmal4G commented Mar 26, 2021 •

edited

Loading

ghost commented Mar 26, 2021

michael-hawker commented Mar 29, 2021

Nirmal4G commented Mar 29, 2021

Rosuavio left a comment

Nirmal4G commented Mar 31, 2021

ghost commented Mar 31, 2021

Rosuavio left a comment

Nirmal4G commented Mar 31, 2021 •

edited

Loading

Rosuavio commented Mar 31, 2021

Nirmal4G commented May 18, 2021

michael-hawker left a comment

michael-hawker May 20, 2021

Nirmal4G commented May 20, 2021

michael-hawker commented Jun 1, 2021 •

edited

Loading

Nirmal4G commented Jun 1, 2021


		<!-- .NET Standard 1.4 doesn't have the Span<T> type, ValueTuple or the [Pure] attribute -->

Clean-up project files and MSBuild logic #3893

Clean-up project files and MSBuild logic #3893

Conversation

Nirmal4G commented Mar 26, 2021 • edited Loading

Fixes #3896

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Other information

ghost commented Mar 26, 2021

michael-hawker commented Mar 29, 2021

Nirmal4G commented Mar 29, 2021

Rosuavio left a comment

Choose a reason for hiding this comment

Nirmal4G commented Mar 31, 2021

ghost commented Mar 31, 2021

Rosuavio left a comment

Choose a reason for hiding this comment

Nirmal4G commented Mar 31, 2021 • edited Loading

Rosuavio commented Mar 31, 2021

Nirmal4G commented May 18, 2021

michael-hawker left a comment

Choose a reason for hiding this comment

michael-hawker May 20, 2021

Choose a reason for hiding this comment

Nirmal4G commented May 20, 2021

michael-hawker commented Jun 1, 2021 • edited Loading

Nirmal4G commented Jun 1, 2021

Nirmal4G commented Mar 26, 2021 •

edited

Loading

Nirmal4G commented Mar 31, 2021 •

edited

Loading

michael-hawker commented Jun 1, 2021 •

edited

Loading