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 DbMetaDataFactory.cs #1278

Merged
merged 4 commits into from
Oct 7, 2021

Conversation

lcheunglci
Copy link
Contributor

Relates to #1261 . I moved the dotnet version of DbMetaDataFactory to the shared src and updated the reference in the netfx project to it because the netcore version uses the newer C# syntax and comments and code has been cleaned up.

@lcheunglci lcheunglci changed the title MergeSNI for DbMetaDataFactory.cs Move into shared for DbMetaDataFactory.cs Sep 22, 2021
@Wraith2
Copy link
Contributor

Wraith2 commented Sep 22, 2021

How do you feel about updating the style of the code? There are lots of modern naming violations in there like these:

        private const string _collectionName = "CollectionName";
        private const string _populationMechanism = "PopulationMechanism";
        private const string _populationString = "PopulationString";
        private const string _maximumVersion = "MaximumVersion";
        private const string _minimumVersion = "MinimumVersion";
        private const string _dataSourceProductVersionNormalized = "DataSourceProductVersionNormalized";
        private const string _dataSourceProductVersion = "DataSourceProductVersion";
        private const string _restrictionDefault = "RestrictionDefault";
        private const string _restrictionNumber = "RestrictionNumber";
        private const string _numberOfRestrictions = "NumberOfRestrictions";
        private const string _restrictionName = "RestrictionName";
        private const string _parameterName = "ParameterName";

And things like if (SupportedByCurrentVersion(row) == false) which aren't wrong as such but it would be nice to get cleaned up. In general when I'm working on a merge I turn on the info messages for the file and try to resolve as many of them according to the repo style as I can.

@lcheunglci
Copy link
Contributor Author

lcheunglci commented Sep 23, 2021

How do you feel about updating the style of the code? There are lots of modern naming violations in there like these:

        private const string _collectionName = "CollectionName";
        private const string _populationMechanism = "PopulationMechanism";
        private const string _populationString = "PopulationString";
        private const string _maximumVersion = "MaximumVersion";
        private const string _minimumVersion = "MinimumVersion";
        private const string _dataSourceProductVersionNormalized = "DataSourceProductVersionNormalized";
        private const string _dataSourceProductVersion = "DataSourceProductVersion";
        private const string _restrictionDefault = "RestrictionDefault";
        private const string _restrictionNumber = "RestrictionNumber";
        private const string _numberOfRestrictions = "NumberOfRestrictions";
        private const string _restrictionName = "RestrictionName";
        private const string _parameterName = "ParameterName";

And things like if (SupportedByCurrentVersion(row) == false) which aren't wrong as such but it would be nice to get cleaned up. In general when I'm working on a merge I turn on the info messages for the file and try to resolve as many of them according to the repo style as I can.

That's a great idea, but perhaps, I'm missing something in my visual studio settings to get these info messages to show up for the file because I currently don't see any warning or info messages in my editor. I was reading in the coding-style.md that there's a .editorconfig, but I'm not sure exactly how it works.

@Wraith2
Copy link
Contributor

Wraith2 commented Sep 23, 2021

If I turn on info messages for this file I get this:

image

and each of those naming rule violations will correspnd to one of the constants. Instead of _collectionName it should not start with an underscore and should start with a capital, so CollectionName

@lcheunglci
Copy link
Contributor Author

Thanks @Wraith2 , I'm able to see the same info messages in the error list panel as in your screenshot. I'll fix them in my next commit.

…essages IDE1006, IDE10059, IDE0051 and IDE0090
@Wraith2
Copy link
Contributor

Wraith2 commented Sep 23, 2021

The CI isn't happy but i don't think it's related to your changes, for example
https://dev.azure.com/sqlclientdrivers-ci/sqlclient/_build/results?buildId=38974&view=logs&j=84415923-4175-5328-da83-e82a5e56172c&t=a9101a3f-4ee2-51f7-d19f-63edc33ee862&l=45 sadly it's usually a bit flaky so you end up having to check each failing leg to identify if it's important or infrastructure related,

@JRahnama
Copy link
Member

The CI isn't happy but i don't think it's related to your changes, for example
https://dev.azure.com/sqlclientdrivers-ci/sqlclient/_build/results?buildId=38974&view=logs&j=84415923-4175-5328-da83-e82a5e56172c&t=a9101a3f-4ee2-51f7-d19f-63edc33ee862&l=45 sadly it's usually a bit flaky so you end up having to check each failing leg to identify if it's important or infrastructure related,

that has happened before. It is due to an unsuccessful database-drop act. I will trigger the tests to rerun and if not going well will do a clean up.

@DavoudEshtehari DavoudEshtehari added the ➕ Code Health Changes related to source code improvements label Sep 30, 2021
Copy link
Member

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

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

Good job 👍

@cheenamalhotra cheenamalhotra added this to the 4.0.0-preview3 milestone Oct 7, 2021
@cheenamalhotra cheenamalhotra merged commit 64befd5 into dotnet:main Oct 7, 2021
@lcheunglci lcheunglci deleted the MergeSNI-DbMetaDataFactory branch October 14, 2021 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ Code Health Changes related to source code improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants