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 namespaces for extension types #3743

Merged
31 commits merged into from
Feb 24, 2021

Conversation

Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Feb 9, 2021

Related to #3422

PR Type

What kind of change does this PR introduce?

  • Refactoring

What is the current behavior?

Quoting from #3422 (comment), we have the current situation with respect to extension methods in the Toolkit:

On a related note - lately I've been thinking a bit about the various extensions in the toolkit in particular.
@mrlacey already pointed this out in the first post in this issue, and I agree that that inconsistency is a bit weird, especially now that newer packages have started to just place them in their respective root namespaces. Basically we have the following situation:

  • Microsoft.Toolkit.Extensions (from Microsoft.Toolkit)❌
  • Microsoft.Toolkit.Extensions (from Microsoft.Toolkit.Diagnostics*) ❌
  • Microsoft.Toolkit.HighPerformance.Extensions
  • Microsoft.Toolkit.Mvvm.Messaging
  • Microsoft.Toolkit.Uwp.Extensions
  • Microsoft.Toolkit.Uwp.UI.Extensions
  • Microsoft.Toolkit.Uwp.UI.Animations
  • Microsoft.Toolkit.Uwp.UI.Media** ✅

What is the new behavior?

This PR moves all the extensions (including XAML markup extensions) to the root namespace of each package.
It also fixes some related issues like inconsistent naming, incorrect method locations, etc.

Open questions

Some extension types now have a number of properties that are only relevant when grouped together, as they refer a specific feature. For instance, all the SurfaceDial properties in TextBoxExtensions, or the ones related to Regex. @michael-hawker mentioned the "option" pattern that the Graph Controls have been using, which would look something like this:

<TextBox>
  <ui:TextBoxExtentions.SurfaceDial>
    <ui:SurfaceDialOptions MinValue="0" MaxValue="10" EnableHapticFeedback="True" ... />
  </ui:TextBoxExtensions.SurfaceDial>
</TextBox>

Do we want to make this change as well while we're at it? Thoughts? 🙂

Additional details

Haven't fully tested all the samples in the sample app just yet, have just focused on ensuring everything built fine for now.
Also, this PR is based on top of #3685, which needs to be merged before this one.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

@ghost
Copy link

ghost commented Feb 9, 2021

Thanks Sergio0694 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 🙌

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Went through about half and provided some initial feedback on a couple of items.

I like trying to see if we can do something with the SurfaceDial API (I have one somewhere I can try to test with if we change things). I'm not sure if SurfaceDial and SurfaceDialOptions are the best names, we haven't actually implemented this pattern yet for the Graph, but it's on our minds: CommunityToolkit/Graph-Controls#67

@michael-hawker michael-hawker added the next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. label Feb 9, 2021
@michael-hawker
Copy link
Member

FYI @Sergio0694, when you create the PR, you can set the base branch to the other branch you had created in the other PR, so that those commits won't show here against the main branch.

After that other PR is merged, we could rebase back off the main branch.

@michael-hawker
Copy link
Member

Not sure if I didn't get to them yet, or if we didn't think about it... what about our Converters in the ...UI.Converters namespace? @Sergio0694 @mrlacey thoughts?

@michael-hawker
Copy link
Member

@Sergio0694 looks like the build failed as unit tests weren't updated with the namespace changes for their usings or something?

@mrlacey
Copy link
Contributor

mrlacey commented Feb 9, 2021

Open questions

Some extension types now have a number of properties that are only relevant when grouped together, as they refer a specific feature. For instance, all the SurfaceDial properties in TextBoxExtensions, or the ones related to Regex. @michael-hawker mentioned the "option" pattern that the Graph Controls have been using, which would look something like this:

<TextBox>
  <ui:TextBoxExtentions.SurfaceDial>
    <ui:SurfaceDialOptions MinValue="0" MaxValue="10" EnableHapticFeedback="True" ... />
  </ui:TextBoxExtensions.SurfaceDial>
</TextBox>

Do we want to make this change as well while we're at it? Thoughts? 🙂

I do like the look of this shorter, simpler, grouped syntax.
Rather than a straight breaking change, could the new options syntax be added as a separate option and the old way marked as deprecated so there's a timeframe for developers to adjust and change existing code?

@mrlacey
Copy link
Contributor

mrlacey commented Feb 9, 2021

Not sure if I didn't get to them yet, or if we didn't think about it... what about our Converters in the ...UI.Converters namespace? @Sergio0694 @mrlacey thoughts?

I'm not sure what you're thinking about checking or changing here.
Everything seems to be consistently in the Microsoft.Toolkit.Uwp.UI.Converters namespace.

The only thing that could be related and sticks out to me is the Microsoft.Toolkit.Converters class. What's bad is that it's a generic name for a class with only one static method and the documentation says it contains a "[s]et of helpers." I'd be happy to discuss giving that a better name.

@Sergio0694
Copy link
Member Author

I do like the look of this shorter, simpler, grouped syntax.
Rather than a straight breaking change, could the new options syntax be added as a separate option and the old way marked as deprecated so there's a timeframe for developers to adjust and change existing code?

@mrlacey I'm wondering whether it's actually worth it to have that considering the massive amounts of breaking changes we have in 7.0 anyway, from whole packages being split to general API changes with no intermediate obsolescence of earlier versions.
I mean, this entire PR is massively breaking per se already 😄
Wouldn't it be possible to just make all these breaking changes now at this point so that devs will only have to do these changes once when 7.0 lands, and then be set for future versions, instead of having to deal with multiple consecutive breaking updates?

@mrlacey
Copy link
Contributor

mrlacey commented Feb 9, 2021

@mrlacey I'm wondering whether it's actually worth it to have that considering the massive amounts of breaking changes we have in 7.0 anyway, from whole packages being split to general API changes with no intermediate obsolescence of earlier versions.
I mean, this entire PR is massively breaking per se already 😄
Wouldn't it be possible to just make all these breaking changes now at this point so that devs will only have to do these changes once when 7.0 lands, and then be set for future versions, instead of having to deal with multiple consecutive breaking updates?

Is there a (generated-hopefully) list of all the breaking changes so far?
I'm not clear the extent of the other breaking changes that have been made. Changing namespaces and having some packages split seems like something tooling can easily help with. Having to manually rewrite code in a different way without prior warning seems more of an imposition. I'm just wondering if there is already precedent here.

@michael-hawker
Copy link
Member

@mrlacey yeah we've been trying to telegraph for a while that 7.0 is going to be a big release with breaking changes. The whole start of my release blog post is going to be calling that out that the release has cleaned and changed a lot of things.

We've been tagging PRs and should have our list of breaking changes. Hopefully for most folks it'll just be some namespace changes or including different packages, except for the couple of things we've removed (like ScrollHeader and the ImageEx caching).

So, I'm ok with us just making straight changes, as we have a bad habit of not cleaning up things later as well. I think the main exception to this is the DispatcherHelper where we know a ton of folks are using it and will need to migrate to DispatcherQueue for WinUI 3. Things like the SurfaceDial helpers are going to be a lot more niche in their usage.

@mrlacey
Copy link
Contributor

mrlacey commented Feb 9, 2021

Ok. I'm happy with a straight change if there's loads of other breaking changes too.

@Sergio0694
Copy link
Member Author

Awesome 😄
@michael-hawker I can go ahead and make those changes once we all agree on the naming and on the new API surface, I guess we can either brainstorm that here or we can sync up at some point to briefly discuss those changes when you have some time 👍

@Sergio0694
Copy link
Member Author

Note for reviewers: I'm aware there are conflicts to be solved, but I'm waiting for #3685 to be merged first to then rebase this PR on top of master directly. This branch is currently based on top of that PR as well, so I'm fine with waiting for that first before solving conflicts and making this PR to date to avoid creating even more conflicts between the three branches 😄

@Kyaa-dost Kyaa-dost linked an issue Feb 17, 2021 that may be closed by this pull request
@Sergio0694 Sergio0694 force-pushed the refactoring/extensions-namespace branch 2 times, most recently from 9905484 to 85fb7da Compare February 22, 2021 20:58
This was referenced Mar 12, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merge ⚡ extensions ⚡ improvements ✨ in progress 🚧 introduce breaking changes 💥 next preview ✈️ Label for marking what we want to include in the next preview release for developers to try.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review namespaces and folder structure coupling
5 participants