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

Changed the namespace of ISettingsStorageHelper interface inline with other helpers. #442

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maroldo26
Copy link

Issue link : #403

Closes #403

Changed the namepspace of the interface ISettingsStorageHelper from CommunityToolkit.Helpers to CommunityToolkit.Common.Helpers

PR Checklist

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • Tested code with current supported SDKs
  • Contains NO breaking changes
  • Code follows all style conventions

Existing code which refers ISettingsStorageHelper using old namespace CommunityToolkit.Helpers will break.

@net-foundation-cla
Copy link

net-foundation-cla bot commented Sep 15, 2022

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@Nirmal4G Nirmal4G left a comment

Choose a reason for hiding this comment

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

This is a straight forward change. I like it but nonetheless, a huge breaking change; yet not many seem to have used it. So, this might be a good candidate for v8.1 preview to see how the community reacts to the change.

@Sergio0694 Thoughts?

@Sergio0694
Copy link
Member

I mean I'd really just like to remove the whole bunch of typed in this namespace entirely, but that's a different topic. The changes here look good to me, though yeah this is a breaking change. I think some libs in Microsoft Graph are using these helpers, so we might have to double check with them first that this won't be too disruptive. @michael-hawker do you know who we'd need to ask, just to double check or at the very least give them a heads up?

@Sergio0694 Sergio0694 added introduce breaking changes 💥 This change would be a breaking change common 💼 Issues/PRs for the Common package labels Oct 1, 2022
@@ -3,7 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using CommunityToolkit.Helpers;
using CommunityToolkit.Common.Helpers;

namespace CommunityToolkit.Common.Extensions;
Copy link
Member

Choose a reason for hiding this comment

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

There was also the question of here if the Extensions should be removed here as no other extensions use this namespace, eh?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be moved to Helpers then?

@michael-hawker
Copy link
Member

Yeah, this got introduced in the 8.0 update and was really just a copy-paste issue, unfortunate we didn't catch it before hand, so now it's a breaking change. I don't think we're planning a 9.0 any time soon.

Considering the WCT stuff will ship after you do an update, probably good to just push it through. Not sure how best to communicate a breaking change in a minor release though. It's really just a mistake to correct what was intended vs. us actually trying to change behavior, but it's still a contractual break to semantic versioning... 🙁 Probably would have been easier if we did an 8.0.1 soon after release or something we had noticed it right away.

@Nirmal4G
Copy link
Contributor

Nirmal4G commented Oct 4, 2022

It's really just a mistake to correct what was intended vs. us actually trying to change behavior...

It probably depends on how many people depend on this directly. I think you can ship a preview to see how many are breaking, even if it's just a few (since, most don't use previews), we can mark this for next major instead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common 💼 Issues/PRs for the Common package introduce breaking changes 💥 This change would be a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ISettingsStorageHelper namespace incorrect
4 participants