-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Make CA1304 & CA1305 warnings #6813
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
base: main
Are you sure you want to change the base?
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6813 +/- ##
==========================================
+ Coverage 55.07% 59.02% +3.95%
==========================================
Files 1934 1934
Lines 85777 85777
Branches 7675 7675
==========================================
+ Hits 47238 50634 +3396
+ Misses 36749 33269 -3480
- Partials 1790 1874 +84 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
withinfocus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lot of whitespace differences I had to temporarily disable, not sure we want those.
|
@withinfocus The The Scim integration test project is because it had CRLF line endings instead of LF defined by our editorconfig so VSCode updated it. I can change it back and update it separately if that is preferred though. |
AmyLGalles
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
sbrown-livefront
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅

🎟️ Tracking
📔 Objective
This makes it a warning (which is upgraded to error by default) to not specify a
CultureInfoorIFormatProviderwhen there is an overload that takes them. The default of .NET is to use the current culture but that can cause differences for people who have a different culture on their computer. Likely most of these warnings should haveCultureInfo.InvariantCulturespecified but that is a difference for what is happening today so a decision has to be made that it is correct for each place.This helps make things like #6811 less likely to happen.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes