-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[EventHubs] Enable the FxCop Analysizers #11423
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
Merged
ellismg
merged 1 commit into
Azure:master
from
ellismg:ellismg/enable-fxcop-for-eventhubs
Apr 22, 2020
Merged
[EventHubs] Enable the FxCop Analysizers #11423
ellismg
merged 1 commit into
Azure:master
from
ellismg:ellismg/enable-fxcop-for-eventhubs
Apr 22, 2020
Conversation
This file contains hidden or 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
ellismg
commented
Apr 17, 2020
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/EventProcessor{TPartition}.cs
Outdated
Show resolved
Hide resolved
ellismg
commented
Apr 17, 2020
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpConsumer.cs
Outdated
Show resolved
Hide resolved
a16a712 to
5373398
Compare
jsquire
approved these changes
Apr 21, 2020
Member
jsquire
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. Thanks for kicking the tires on this. The analyzer spam seems far less plentiful than the last time we thought to try and enable it. Not a fan of the #pragma blocks that we need, but I'd agree its the lesser of evils.
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpConsumer.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/EventHubsRetryOptions.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/EventProcessor{TPartition}.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/EventProcessor{TPartition}.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/EventProcessor{TPartition}.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/EventProcessor{TPartition}.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/PartitionReceiver.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/PartitionReceiver.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/PartitionReceiver.cs
Outdated
Show resolved
Hide resolved
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.
5373398 to
58fc47a
Compare
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.
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.