[EventHubs] Use Invariant vs Current culture #11414
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
By default, the `.Parse` and `.TryParse` methods, as well as `.ToString` use the current culture. In some of our uses, we don't want to the behavior to differ based on the culture of the local machine.
|
In practice, I'm not sure this actually matters because of the narrow scope of our use of these APIs. But this is reasonable hygiene. |
jsquire
approved these changes
Apr 17, 2020
ellismg
added a commit
to ellismg/azure-sdk-for-net
that referenced
this pull request
Apr 21, 2020
As a follow up to Azure#11414, I was trying to understand why did not see errors from the "FxCop Analyiszers", which would have pointed out that issue to us. The reason was simple - we had disabled the analysis for these projects. I've re-enabled them, either fixing up issues or adding suppressions. We ended up catching some reasonable errors (a few cases where we called the ArgumentException constructor but messed up the order of the message and the parameter name) as well as a case where we were passing two parameters to an internal method but then disregarding them. In practice, the code we had was still "correct", because it happened to only be called from a single place today and the only caller was passing the same values that the implementation was actually using. Overall, I think taking this would be worthwhile. It brings this library back in line with the the others. I could imagine some library wide supressions of rules here, specifically the one about not calling virtual methods from constructors, since in practice we are unlikely to hit the problems the rule intends to guard for, but for now I've just made the diff look "as bad as possible" by doing supressions around the offending lines with justifications.
ellismg
added a commit
that referenced
this pull request
Apr 22, 2020
As a follow up to #11414, I was trying to understand why did not see errors from the "FxCop Analyiszers", which would have pointed out that issue to us. The reason was simple - we had disabled the analysis for these projects. I've re-enabled them, either fixing up issues or adding suppressions. We ended up catching some reasonable errors (a few cases where we called the ArgumentException constructor but messed up the order of the message and the parameter name) as well as a case where we were passing two parameters to an internal method but then disregarding them. In practice, the code we had was still "correct", because it happened to only be called from a single place today and the only caller was passing the same values that the implementation was actually using. Overall, I think taking this would be worthwhile. It brings this library back in line with the the others. I could imagine some library wide supressions of rules here, specifically the one about not calling virtual methods from constructors, since in practice we are unlikely to hit the problems the rule intends to guard for, but for now I've just made the diff look "as bad as possible" by doing supressions around the offending lines with justifications.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By default, the
.Parseand.TryParsemethods, as well as.ToStringuse the current culture. In some of our uses, we don't want to the
behavior to differ based on the culture of the local machine.