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

Replace SecurityHelper.AreStringTypesEqual with string.Equals for code quality #9521

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

h3xds1nz
Copy link
Contributor

@h3xds1nz h3xds1nz commented Aug 3, 2024

Description

One of the few leftover remnants from CAS times is the SecurityHelper.AreStringTypesEqual that just calls case-insensitive string.Equals. There is no reason for this method as it has even lost the original comment throughout the removals and just makes newcomers wonder why is it there.

Customer Impact

Few bytes less in size of all libraries.

Regression

No.

Testing

Local build.

Risk

None.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners August 3, 2024 23:34
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Aug 3, 2024
@lindexi
Copy link
Member

lindexi commented Aug 7, 2024

I think the SecurityHelper.AreStringTypesEqual will be inline

@h3xds1nz
Copy link
Contributor Author

h3xds1nz commented Aug 7, 2024

Yes, it would be inlined, of course. The reason for this is purely code quality. There should be no SecurityHelper whatsoever because there is no CAS anymore. This was planned to be removed by the team but after handover it has never happened., you may find the discussion in older issues/PRs regarding CAS.

It only complicates the code readibility with another level of indirection. Oh, look, "AreStringTypesEqual"... but it is CI compare!

@lindexi
Copy link
Member

lindexi commented Aug 7, 2024

there is no CAS anymore

Yeah, I fully agree.

@dipeshmsft dipeshmsft self-assigned this Oct 8, 2024
@dipeshmsft dipeshmsft merged commit 188112f into dotnet:main Oct 8, 2024
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants