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

Refactor remaining Project and Build files #85

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Nirmal4G
Copy link
Contributor

@Nirmal4G Nirmal4G commented Jan 11, 2022

Info

Extracted from CommunityToolkit/WindowsCommunityToolkit#4234 as the Common Code projects were separated in this new repository.

Changes

Simplify Roslyn multi-targeting

  • Refactor Roslyn multi-targeting to use multiple projects
    in the same project directory without using Shared projects.
  • This refactoring is made in preparation for the solution to use
    the NuGet's CPVM and TargetFramework as alias features.
  • This also slightly improves IDE load time and Build performance.

Move MSBuild logic into new files

  • Move the T4 MSBuild logic to a new file.
  • Move Public Key to a new file toolkit.spk.
  • Move MSBuild logic to near-by Directory.Build.{props|targets} files.

Simplify MSBuild logic in project files

  • Add necessary guard to check for pack.
  • Remove redundant properties and values.
  • Remove and adjust quotes in property functions.
  • Use wildcards to generalize and reduce items declared.

Add, Update and Format build Comments

  • Remove redundant comments.
  • Add missing comments on certain code blocks.
  • Format code in comments with proper quote style.

Misc Changes

  • Fix the text flow warping in the MSBuild Console logging.
  • Use checked version properties instead of hard-coded checks.
  • Update the structure of the projects list in the Solution file.

PR Checklist

  • Created a feature branch in your fork
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main)
  • Header has been added to all new source files (ran build/Update-Headers.ps1)
  • Tested code with current supported SDKs
  • Contains NO breaking changes
  • Code follows all style conventions

Notes

Please suggest alternate names for dotnet Community Toolkit.sln. We have already established that we can't use .NET.
I tried using NET, DotNET, dotNET and Core as prefix. Except for Core, others neither look good nor stand-out in the file list.

  • Rebase merge please.
  • When merging, please update the commit title to PR title instead of the default Merge pull request #xxxx from repo/branch, and commit message to either PR message or messages of individual commits. The auto-merge option does this by default.

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Jan 11, 2022

Project File structuring

  • SOF (Start Of File) Imports,
  • Core properties, Package properties, build properties,
  • Build items, Resource items, Content items, Misc. items,
  • In-place Imports, Choose (elements within follows outer sort order)
  • ProjectReference items, PackageReference items,
  • Targets and Properties and Items close to said Targets,
  • EOF (End Of File) Imports.

Where there's a condition by target properties, we should group them together under Choose. For multiple target values, we should sort them by the order in which they are defined. And the order in which they should be defined is either ascending or descending in terms of compatibility layering.

@Nirmal4G Nirmal4G force-pushed the hotfix/refactor branch 4 times, most recently from f7299f7 to ddcd107 Compare January 19, 2022 18:34
@Nirmal4G Nirmal4G force-pushed the hotfix/refactor branch 3 times, most recently from a5bd710 to 8002aaa Compare January 25, 2022 16:23
@Nirmal4G Nirmal4G force-pushed the hotfix/refactor branch 2 times, most recently from 82a70b6 to fbeb0c1 Compare April 17, 2022 06:43
@Nirmal4G Nirmal4G force-pushed the hotfix/refactor branch 2 times, most recently from 1ab0a31 to 2eb009b Compare April 19, 2022 11:39
@danielbanda danielbanda mentioned this pull request May 4, 2022
12 tasks
@Nirmal4G Nirmal4G force-pushed the hotfix/refactor branch 2 times, most recently from 9754305 to 51e244a Compare May 17, 2022 10:11
@danielbanda danielbanda mentioned this pull request May 25, 2022
12 tasks
@Nirmal4G
Copy link
Contributor Author

@Sergio0694 PR closed by 237 by mistake. Can you reopen it?

@Sergio0694
Copy link
Member

Huh, weird, yeah no problem! 😄

@Sergio0694 Sergio0694 reopened this May 25, 2022
Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Left some nits 🙂

.gitignore Outdated Show resolved Hide resolved
CommunityToolkit.Mvvm/CommunityToolkit.Mvvm.targets Outdated Show resolved Hide resolved
Directory.Build.props Show resolved Hide resolved
build/Community.Toolkit.Common.targets Outdated Show resolved Hide resolved
@Nirmal4G Nirmal4G force-pushed the hotfix/refactor branch 2 times, most recently from 456db4d to d648a69 Compare June 3, 2022 17:19
@Nirmal4G Nirmal4G requested a review from Sergio0694 June 3, 2022 18:13
@Nirmal4G Nirmal4G marked this pull request as ready for review June 23, 2022 17:32
@Nirmal4G
Copy link
Contributor Author

@Sergio0694 Updated in response to PR #428 which broke this!

@Sergio0694
Copy link
Member

Just a heads up, there's major changes I'm doing in #436, so this PR will need updates after that as well.

@Nirmal4G Nirmal4G marked this pull request as draft October 1, 2022 17:08
@Nirmal4G Nirmal4G force-pushed the hotfix/refactor branch 5 times, most recently from 404c5be to c532ba6 Compare November 27, 2022 21:32
@Nirmal4G Nirmal4G force-pushed the hotfix/refactor branch 3 times, most recently from 8ada70e to 6210e79 Compare January 13, 2023 10:18
@michael-hawker
Copy link
Member

Still going to suggest that we close this PR, as it should be broken out into smaller PRs as mentioned above.

@Nirmal4G in general doing things like renaming a directory from build to eng has little purpose or value to the project and just creates noise in these PRs. It's something that should be discussed in an issue first, folks can agree on a timing and value of the proposal, and then it can be coordinated with others working on the repo for a major folder restructure between releases.

Similarly, unless we can automate structuring of the project file XML with a tool like XAML Styler does for XAML files in the WCT repo, then manually moving Item/PropertyGroups around is just a temporary thing that also just creates more noise that's hard to review. Especially when mixed in with other changes in part of a larger PR.

Diving into making changes can be fun and exciting, but if you want to ensure your contributions are having a meaningful impact and will be merged; it's best to start a conversation with an issue or discussion first to lay out exactly what you plan to do, how it benefits the project, and what's involved with the change. This should be scoped to something that's a single 'block' and not a laundry list of lots of items. Each idea should be its own issue.

Or ask to pick up an already open bug or issue on the repo that someone's not working on or reach out on Discord to Sergio or myself and ask if there's something on the top-of-our-minds build related that we're struggling with that you could assist with. Really happy to have your passion directed at our projects, but we just want to ensure we're on the same page and can move forward together! Thanks!

@Nirmal4G
Copy link
Contributor Author

@michael-hawker This is still in draft! Also this is on top of existing PRs. Changes that are mutually exclusive are separated but when they are cumulative, how they can be separated?

Code changes are one thing but refactorings are if not always cumulative and to produce clean diff and blame, they need to be arranged properly by theme of change or purpose of change.

I'm still a novice but if may ask, how would you separate this patch without causing conflicts that break build!?

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Jan 14, 2023

@michael-hawker There's a discussion open for these PRs. It's part of a larger refactoring with the goal of introducing a common build infra for Community Toolkit repos (or atleast for my forks if we're not moving forward with the idea!).

  1. [Build] Centrally manage NuGet package versions WindowsCommunityToolkit#4691
  2. Improve build infra for .NET Community Toolkit repository #463

- Rename `build` folder to `eng`:
  - This is a standard build infra directory used in official dotnet projects.

- Rename NuGet Icon to `Icon.png`:
  - This is no longer used as a public reference point for NuGet icon URL.
  - Also, Icon URL is deprecated. Hence, it's safe to change.

- Normalize casing for `ReadMe.md`:
  - Repository information files such as ReadMe, License, etc... are only UPPER_CASE
    if they are without an extension. With extension, the casing becomes PascalCase
    or Kebab-Case. The primary reason is attention to the presentation of file names.
  - Do Kebab-Case when a phrase is presented. E.g., `Code-of-Conduct.md`.

- Rename solution file to `CommunityToolkit.sln`:
  - The `dotnet` seems implied and also doesn't stand-out in the file list because of the lower casing and `d` char.
  - Spaces are a main issue when doing automation (_like using `*.sln` in build scripts and in URLs it adds `%20`_).

- Move `toolkit.snk` file to `eng` sub-directory.
- Remove un-needed and deleted files from solution.

- Update Git Ignore entries to latest from upstream.
- Indent text in `ThirdPartyNotices.txt` with spaces instead.
- Fix the text flow warping in the MSBuild Console logging.
- Use checked version properties instead of hard-coded checks.
- Update the structure of the projects list in the Solution file.

- Refactor Roslyn multi-targeting to use multiple projects
  in the same project directory without using Shared projects.
  This refactoring is made in preparation for the solution to use
  the NuGet's CPVM (Central Package Versions Management) feature.
  This also slightly improves IDE load time and Build performance.
- Move the T4 MSBuild logic to a new file.
- Move Public Key to a new file 'toolkit.spk'.
- Move MSBuild logic to near-by `Directory.Build.{props|targets}` files.
Follow the following rules:

- SOF (_Start Of File_) Imports,
- Core properties, Package properties, build properties,
- Build items, Resource items, Content items, Misc. items,
- In-place Imports, Choose (_elements within follows outer sort order_)
- `ProjectReference` items, `PackageReference` items,
- Targets and Properties and Items close to said Targets,
- EOF (_End Of File_) Imports.

Where there's a condition by target properties, we should group them together under `Choose`.
For multiple target values, we should sort them by the order in which they are defined.
And the order in which they should be defined is either ascending or descending in terms of compatibility layering.
- Add necessary guard to check for pack.
- Remove redundant properties and values.
- Remove and adjust quotes in property functions.
- Use wildcards to generalize and reduce items declared.
- Remove redundant comments.
- Add missing comments on certain code blocks.
- Format code in comments with proper quote style.
Some property groups have conditions that also self-explain their purpose.
So, Add labels to bare property groups only to differentiate among themselves.
Then, when contributors add any additional properties, they'll know where to put them.
@wzxprince
Copy link

I am Chinese!!!I like it!

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