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

Add some polyfill to simplify code across compilations #3974

Merged
merged 2 commits into from
Aug 29, 2022

Conversation

Evangelink
Copy link
Member

Description

Add some polyfill to simplify code across compilations. This way we can use only one API but correctly target the recommended way for newer .NET.

@Evangelink Evangelink enabled auto-merge (squash) August 26, 2022 09:45
/// <summary>
/// A polyfill helper for Guid.
/// </summary>
internal static class GuidPolyfill
Copy link
Member Author

Choose a reason for hiding this comment

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

@MarcoRossignoli Was suggesting to have only one class called Polyfill that would host all polyfills as sub-classes (e.g. Polyfill.Guid.Parse. It looks like a good idea but doesn't allow for extension methods (need to be defined at top class-level).

We can:

  1. Keep solution implemented in this PR
  2. Go with @MarcoRossignoli's suggestion for non-extension methods (not ideal as it would force us to have 2 ways)
  3. Same as 2. but we do that also for extension methods (will no longer be extension methods).

Copy link
Member

Choose a reason for hiding this comment

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

I see theoretical way around it but it requires three classes and instances. Seems like PolyfillGuid or GuidPolyfill in separate class is much easier way to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's ok as-is to me.

#else
Id = Guid.Parse(guidString);
#endif
Id = GuidPolyfill.Parse(guidString, CultureInfo.InvariantCulture);
Copy link
Contributor

@MarcoRossignoli MarcoRossignoli Aug 26, 2022

Choose a reason for hiding this comment

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

should the used signature be the "older" one, I mean from user perspective looks like the culture is important...but it's important only for one version...so I wonder if we should have the one without culture and setup the correct default for the newer one.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good suggestion. I don't see us using anything else than InvariantCulture so it would simplify calls. My idea here is to match newest API so that code looks like what you would expect if using recent .NET.

Maybe I will keep it as-is.

@Evangelink Evangelink enabled auto-merge (squash) August 28, 2022 13:45
@Evangelink Evangelink enabled auto-merge (squash) August 29, 2022 09:54
@Evangelink Evangelink merged commit 29dcc0d into microsoft:main Aug 29, 2022
@Evangelink Evangelink deleted the polyfills branch August 29, 2022 09:56
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.

3 participants