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 #4234

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

Conversation

Nirmal4G
Copy link
Contributor

@Nirmal4G Nirmal4G commented Sep 8, 2021

Changes

Misc refactors (Follow up of #3894, #3896 and #4068)

  • Update variables names and message text.
  • Update comments and fix casing, spelling mistakes.
  • Rearrange lines for better diff for upcoming patches.

Update Notifications package

  • Add ReadMe to Notifications package.
  • Add ThirdPartyNotices.txt to the package.
  • Place the ReadMe, Icon, License and Notices in the root of the package.

Fixup target UAP versions (non-breaking)

  • In both Notifications and XAML Islands Unit Tests projects, We already package them up with lowest UAP version anyway. But the targets don't represent that. So, Fix the project and fallback targets with lowest compatible version instead of latest version.

Update ~OutputPath properties in projects

  • Use Base~ output paths where possible.
  • Use Count() item function where possible.
  • Add missing OutputPath property to WAP projects.

PR Type

What kind of change does this PR introduce?

  • Code style update (formatting only)
  • Refactoring (no functional changes, no API changes)
  • Build or CI related changes (non-breaking)

PR Checklist

Please check if your PR fulfills the following requirements:

  • Created a separate branch (other than main/master) in your fork
  • Based off latest main branch of toolkit
  • Pull Request doesn't include merge commits
  • Header has been added to all new source files
  • Contains NO breaking changes
  • Code follows all style conventions

Other information

  • Please Rebase merge if possible.
  • Please update the default message option to Default to pull request title and description in the Repository Pull Request settings to have a better commit message instead of Merge pull request #xxxx from repo/branch generic message.

@ghost
Copy link

ghost commented Sep 8, 2021

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 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Rosuavio September 8, 2021 20:36
@ghost ghost added the improvements ✨ label Sep 8, 2021
@michael-hawker
Copy link
Member

@Nirmal4G there's a whole bunch of different types of changes here, some seem functional with respect to the targets of the Notifications.

There's not much more value in fixing comments in project files or re-organizing them. If there are things which can be removed because they're in common files great, but lets do that as a separate PR. Same if there are fixes that should be done for the notifications or something (though I don't think we've seen any issues at the moment).

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Sep 9, 2021

@michael-hawker

I'm isolating the changes from #4181 that are not related to CPVM feature to figure out why some tests are failing there.

I can open a separate PR if that's what you want but... The notifications change is NOT breaking or intrusive or even a public change.

So, Can you please reopen this?

@michael-hawker
Copy link
Member

The main thing that I was worried about for the Notifications was the changes to the versioning in the paths as I don't know how that effects the final package output.

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Sep 9, 2021

@michael-hawker Don't worry, it doesn't affect the packaging. I assure you; this PR doesn't contain any breaking change!

@michael-hawker
Copy link
Member

@Nirmal4G I've re-opened so you can re-run the CI. Just want to be weary still of the more nitpicky updates to project files to reduce churn for little value, as we have discussed previously.

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Sep 9, 2021

Sure, I have reduced unnecessary changes. These changes do flow through the next PR.

I have a separate branch like winui for both UWP Toolkit and Desktop Toolkit repos that contains my final patches. Occasionally, I compare them and create necessary patches for upstream. I'll only create patches that I think would be beneficial.

For example, when you want to update the Notification project for newer UAP target, You'd have to update multiple places but now I've made it so that when you update the target version in the common props will update everywhere.

For the code style update for MVVM source generators project file, I followed what I had done in previous PR. I'm still working on the automation of code style analysis but in the meantime, I'll update the new files when possible.

@Nirmal4G Nirmal4G force-pushed the feature/refactor branch 3 times, most recently from 2cc0727 to 1457982 Compare September 10, 2021 13:27
@Nirmal4G Nirmal4G marked this pull request as ready for review September 10, 2021 16:47
@michael-hawker michael-hawker added this to the 7.2/8.0? milestone Sep 21, 2021
@michael-hawker
Copy link
Member

Thanks @Nirmal4G! We're just wrapping up the 7.1 release this week; so it'll take us a little bit to get to this one. I've flagged it for the next milestone for tracking.

@Nirmal4G
Copy link
Contributor Author

@michael-hawker I guess it's okay but this patch only contains non-functional changes. It also does not modify build outputs. That's also one of the reasons why I separated this from the CPVM patch tree, so that we can merge this in sooner than latter.

If possible, I would like to merge this in along with the media controls' DesignTools update for the 7.1 release. But even I understand it'll be a stretch!

I'll be opening a PR soon for that, hopefully by tomorrow.

@michael-hawker
Copy link
Member

Yeah, we're just in a state now where we're touching as little as possible. Will be easier to get this reviewed after release and in to bake while we're working on the next milestone. Just don't want to accidently introduce an issue if we happen to overlook something this close to release when it's even tangentially related to the build/project structure.

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.

Could you please undo all changes affecting the .NET projects? We've paused work there until the split as we don't want PRs to get stuck during the transition. We'll be doing the split soon and this means that we'd either have to rush merging this before then (which I don't want, also because there are a number of changes here I wouldn't approve), or you'd end up with a PR trying to modify a whole lot of files that would then be completely gone from this repository, leaving you with a ton of conflicts.

Let's just keep this isolated to stuff that will remain here (ie. all the UWP stuff) and let's eventually look into this again in the future once the new repository is live. Also FYI, you might want to discuss the changes you'd like to do before actually doing the work to open a PR, otherwise you might end up just wasting time for something that won't be merged anyway. For instance, I don't want any of those file renames/moves you've been doing to the .NET projects (eg. the files ported from the runtime).

Thanks! 🙂

@Nirmal4G
Copy link
Contributor Author

...there are a number of changes here I wouldn't approve...

These are all non-functional changes. Most of them are removals of redundant code. You can check the summary of individual commits on why the changes been made.

As I mentioned before, they are leftovers from previous refactor PRs. Since I was trying to make them diffable with comments (on changes), I had to separate them from those previous PRs.

@Nirmal4G
Copy link
Contributor Author

…I don't want any of those file renames/moves you've been doing to the .NET projects…

Is there any reason why you don't want these files in a predictable location?

@michael-hawker
Copy link
Member

@Nirmal4G really appreciate your enthusiasm, still deciding if we'll have another 7.1.x hotfix, and I'd want this to come in after we do that and start the next cycle, which we've been pushing back a bit. We may still merge #4395 before this though.

@Nirmal4G Nirmal4G changed the base branch from main to rel/7.1.2 December 3, 2021 02:24
@Nirmal4G Nirmal4G marked this pull request as draft January 11, 2022 10:21
@Nirmal4G Nirmal4G changed the base branch from rel/7.1.2 to main January 11, 2022 10:22
@Nirmal4G Nirmal4G marked this pull request as ready for review January 11, 2022 12:57
@Nirmal4G
Copy link
Contributor Author

This is finally done. It took a while after #4395 since I had to separate my local repo to match the remote. A lot of my patches lead to lot of conflicts for these kinds of changes!!

@Nirmal4G Nirmal4G force-pushed the feature/refactor branch 2 times, most recently from 6014481 to 2b22c9f Compare April 17, 2022 09:46
@Nirmal4G Nirmal4G force-pushed the feature/refactor branch from 2b22c9f to 9c255bf Compare May 17, 2022 10:51
@Nirmal4G Nirmal4G marked this pull request as draft June 11, 2022 19:02
- Remove BOM from new files.
- Remove redundant xml tags and attributes.
- Update Git ignores and attributes.
- Ensure to preserve encoding of `UTF-16LE` files.
- Update Toolkit Icons and use `Icon.png` name instead.
- Rename solution, scripts and NuGet config files.
- Remove leftover files from previous refactors.
- Format Notifications package ReadMe.
- Remove redundant code from project files.
- Update Comments and fix mistakes.
- Remove the empty Pack target from all project files.
- Put the empty Pack target in `Directory.Build.targets`.
- Rearrange lines so that upcoming patches will have better diff.
  It will also aid in blame as clean diff makes understanding code easier.
- Also Add `ThirdPartyNotices.txt` to the package.
- Place the ReadMe, Icon, License and Notices in the root of the package.
We already package them up with lowest version anyway. But the targets don't represent that.
So, Fix the project and fallback targets with lowest compatible version instead of latest version.
- Use `Base~` paths where possible.
- Remove redundant `OutputPath` entries.
- Use `Count()` item function where possible.
- Add missing `OutputPath` property to WAP projects.
- Everyone's on x64-bit system. So, It's time switch the default.
- This only changes the default for AnyCPU solution target where
  architecture-agnostic target may not be available.
@Nirmal4G Nirmal4G force-pushed the feature/refactor branch 6 times, most recently from 54e7d0c to d63ce34 Compare October 28, 2022 10:05
@Nirmal4G Nirmal4G force-pushed the feature/refactor branch 2 times, most recently from 6de3996 to 66e14e4 Compare November 27, 2022 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants