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

Move into Shared for SqlCommandBuilder.cs #1284

Merged
merged 13 commits into from
Oct 13, 2021

Conversation

lcheunglci
Copy link
Contributor

Relates to #1261 . I merged the SqlCommandBuilder.cs from netfx into netcore and then move it into the shared src. The netfx version uses System.ComponentModel and some attributes from it, which I assume were for WPF or WinForms and there were some DEBUG ifdefs, so I ifdef'ed them for NETFX.

@Wraith2
Copy link
Contributor

Wraith2 commented Sep 24, 2021

I'm pretty sure we want the designer attributes in all builds. The difference may have come from the early version of netcore not having those attributes but since we don't support anything before 3.1 now if those attributes are present we should keep them.

The only problem you might have is if the resources they use are localized and in that case we'll have to see what the MS team want to do, I know there's a long term goal for getting the netfx localized resources working in netcore.

@lcheunglci
Copy link
Contributor Author

lcheunglci commented Sep 24, 2021

That's good to know. In that case, I should wait for #1272 and merge those in this PR before I remove the ifdefs for the designer attributes

@Wraith2
Copy link
Contributor

Wraith2 commented Sep 24, 2021

It'll depend on the attribute, if you check them on apisof.net for example DesignerSerializationVisibilityAttribute you can see it's in NS2.0 and netcore 2 onwards so it should be ok to do now. Perhaps just take out the iffdefs locally and see if it compiles?

…utes as it is included in .NET Standard 2.0. Add the resource string SqlCommandBuilder_DataAdapter in StringHelper so it compiles on netcore as well without the ifdef NETFX
@lcheunglci
Copy link
Contributor Author

Thanks @Wraith2 , it looks like majority of the attributes builds fine with the exception of ResDescriptionAttribute(StringsHelper.ResourceNames.SqlCommandBuilder_DataAdapter) where the string doesn't exist in the StringHelper in the netcore project. I wonder if it's ok to copy it out of the Strings.resx from netfx as a string literal or should I leave it with the ifdef NETFX

@Wraith2
Copy link
Contributor

Wraith2 commented Sep 24, 2021

Hmm, have a look at #1152 and see if there's any precedent there that can help. I'd say just add the resource.

@DavoudEshtehari DavoudEshtehari added the ➕ Code Health Issues/PRs that are targeted to source code quality improvements. label Sep 30, 2021
@DavoudEshtehari DavoudEshtehari added this to the 4.0.0-preview3 milestone Oct 5, 2021
@DavoudEshtehari DavoudEshtehari merged commit 7583486 into dotnet:main Oct 13, 2021
@lcheunglci lcheunglci deleted the MergeShared-SqlCommandBuilder branch October 14, 2021 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ Code Health Issues/PRs that are targeted to source code quality improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants