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

Update CA1806 to allow user defined methods #3483

Merged
merged 7 commits into from
Apr 27, 2020

Conversation

Evangelink
Copy link
Member

Fix #3479

@Evangelink Evangelink requested a review from a team as a code owner April 7, 2020 13:14

Configurable Rules: [CA1806](https://docs.microsoft.com/en-us/visualstudio/code-quality/CA1806)

Option Values: Names of additional methods (separated by '|') for CA1806.
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to also allow types? Same question for namespaces? IMO types could make sense but I fail to see any point for namespaces.

@codecov
Copy link

codecov bot commented Apr 7, 2020

Codecov Report

Merging #3483 into master will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3483      +/-   ##
==========================================
+ Coverage   95.10%   95.27%   +0.16%     
==========================================
  Files        1053     1046       -7     
  Lines      238158   235810    -2348     
  Branches    15498    15262     -236     
==========================================
- Hits       226510   224667    -1843     
+ Misses       9929     9464     -465     
+ Partials     1719     1679      -40     

@@ -1436,4 +1436,7 @@
<data name="EventsShouldNotHaveBeforeOrAfterPrefixTitle" xml:space="preserve">
<value>Events should not have 'Before' or 'After' prefix</value>
</data>
<data name="DoNotIgnoreMethodResultsMessageUserDefinedMethod" xml:space="preserve">
<value>'{0}' calls '{1'} but does not use the value the method returns. This method is defined as a user-option. Use the result in a conditional statement, assign the result to a variable, or pass it as an argument to another method.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<value>'{0}' calls '{1'} but does not use the value the method returns. This method is defined as a user-option. Use the result in a conditional statement, assign the result to a variable, or pass it as an argument to another method.</value>
<value>'{0}' calls '{1}' but does not use the value the method returns. This method is defined as a user-option. Use the result in a conditional statement, assign the result to a variable, or pass it as an argument to another method.</value>

Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering why the test framework did not fail the tests when 2 arguments are being provided for expected diagnostics?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep that's weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok so the test framework is not failing when extra arguments are provided. {1'} is not parsed properly and so not considered as a placeholder. The .WithArguments() can take more arguments that required and won't fail. Might be worth improving this behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sharwell for the suggestion. Possibly file a suggestion issue on roslyn-sdk repo for the testing library?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -132,6 +132,11 @@
<target state="translated">{0} volá {1}, ale nepoužívá hodnotu, kterou tato metoda vrací. O metodách Linq se ví, že nemají vedlejší účinky. Použijte výsledek v podmíněném příkazu, přiřaďte výsledek proměnné nebo ho předejte jako argument jiné metodě.</target>
<note />
</trans-unit>
<trans-unit id="DoNotIgnoreMethodResultsMessageUserDefinedMethod">
<source>'{0}' calls '{1'} but does not use the value the method returns. This method is defined as a user-option. Use the result in a conditional statement, assign the result to a variable, or pass it as an argument to another method.</source>
Copy link
Contributor

Choose a reason for hiding this comment

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

@Evangelink You would need to rebuild locally to also update the xlf files :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦

@mavasani mavasani merged commit 5ce9fe7 into dotnet:master Apr 27, 2020
@Evangelink Evangelink deleted the CA1806-user-option branch April 27, 2020 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CA1806: Allow for user defined use method result
2 participants