14
\$\begingroup\$

Around the time of writing this post, this question was asked: Extension method to list enum values. The OP is asking how to "fix" their code so a specific warning doesn't pop up (emphasis added):

However, when I compile the code, I get the following warning:

CA1004 Consider a design where 'EnumExtensions.SelectListFor()' doesn't require explicit type parameter 'T' in any call to it.

Can I somehow change the above code to overcome this warning? Or should I just suppress the warning?

The code supplied in the question works just fine, but they aren't really asking for a review. They're more or less asking how to fix a specific issue, which belongs more on Stack Overflow than Code Review.

What should we do here, and with questions like this in the future?

\$\endgroup\$

6 Answers 6

16
\$\begingroup\$

The same thing we do when someone asks us to optimise a specific section of code: We review any and all facets of the code.

I wouldn't strictly say we ignore their comment so much as we're not beholden to their wishes. - @SuperBiasedMan

One of the rules of CR is that as long as code is working as expected, we are expected to review any and all facets of the code - including facets that the OP doesn't ask us to focus on.

The given warning itself is not a code-breaking warning and the code both works and compiles as it is expected to.

There was an interesting (somewhat heated) discussion in the 2nd monitor about whether or not we should treat warnings as errors and I think unfortunately this is a rule that would require context. In the past we have closed questions because they contain warnings, however, not all warnings in all languages are created equal.

  • The question being discussed in this topic is in C#. C# does not have a lot of focus on eliminating warnings and they are considered to be more advisory comments than anything else.
  • C++ (in the question I have linked) commonly is compiled under gcc -Wall, which treats all warnings as errors. In addition, the warnings in the linked question concern actual runtime considerations - the warnings in the C# one are a stylistic concern.

Context is important and there's no way for us to realistically sit down and say "this warning is auto-close, this warning is okay", so I think when it comes to compiler warnings we should be using our best judgement instead of kneejerk closing them.

\$\endgroup\$
3
  • \$\begingroup\$ I agree to ignore the request for specific feature and go ahead reviewing. But the question should be revised to make it a better fit, ideally by the OP. It can be as simple as rephrasing to "I'm particularly interested in X, but I welcome improvement suggestions for any and all facets". When faced with the options of closing or rephrasing, I think OP will logically go for rephrasing. \$\endgroup\$
    – janos
    Jan 13, 2016 at 15:05
  • 1
    \$\begingroup\$ @janos Ideally the question should be made a better fit yes, but what if the question isn't changed? And how does this compare with a certain 100 is a magic number-question? In my opinion, both are on-topic, despite being quite specific questions. \$\endgroup\$ Jan 13, 2016 at 15:45
  • 1
    \$\begingroup\$ @SimonForsberg if OP refuses to make it better, then so be it. It's up to each reviewer if they will want to answer such questions, side-stepping the specific part of the question or not, answering or not. As for 100-is-a-magic-number, yes, that one also asks for something specific, much like the question we're discussing here \$\endgroup\$
    – janos
    Jan 13, 2016 at 18:41
10
\$\begingroup\$

First of all, the question is not off-topic. It doesn't violate our on-topic rules. It is valid to review any and all aspects of the posted code, so even if OP has a specific request, you may side-step that part of the question.

However, asking for something very specific like in this example often confuses reviewers, and may deter them from giving a usual overall review. It would be best if OP could be convinced to rephrase. Which could be as simple as:

I'm especially interested in getting rid of this warning (or should I suppress?), but I welcome improvement suggestions for other aspects too.

And if you suggest this in a comment, asking nicely, OP usually heeds the good advice, and the question overall becomes less confusing, attracts more reviews, and less downvotes.

If OP still wants an answer to something very specific, he might have better luck on Stack Overflow. It's ok to cross post, and rephrase the question on Stack Overflow to follow their on-topic rules, especially including an MCVE.

If OP refuses to rephrase the question for CR, then so be it. If the question is confusingand doesn't follow the form most reviewers are used to, OP might loose some potential reviewers, but that's his problem.

\$\endgroup\$
4
\$\begingroup\$

I think asking "What should we do about questions asking to “fix” warnings?" is the wrong question to ask. To me, it boils more down to this question instead:

Should we treat all questions which mention warnings in the same manner?

I think when rephrased that way, it makes it much more evident what the answer, I feel, should be:

It depends.

This is specifically why we have the tag on Meta.CodeReview. As it seems that there is no possibility that everyone can all agree on whether or not the quoted question is off-topic, and if so, why it is off-topic, I feel it would be a mistake if we, as a community, set a precedent for treating every question that mentions a warning (or "code review error", as phrased in this question) or another such specific concern with a rigid rule.


There are a number of things to keep in mind when evaluating whether or not a question is a good fit, especially when we are not certain. Sometimes it is better to abstain from voting than to create a precedent that will later potentially become an obstacle to every other future question which has similar features. Perhaps a few questions to ask oneself, when uncertain about a question, are:

  • Am I a subject matter expert on the topic?

  • If not, is there a subject matter expert available who could help to clarify?

  • Is there anything the author of the question could do to reduce uncertainty about their question, such as clarify their intention (are they only looking to fix that particular warning, or do they want all aspects of their code reviewed?)

I feel it is better to use the tag on Meta than to try to fit all cases into a rigid rule.

\$\endgroup\$
3
\$\begingroup\$

There's nothing wrong with this question. The compiler warning basically says this:

EnumExtensions.SelectListFor<BrochureTypes>()

Is bad because the compiler can't infer the generic type parameter from usage, which forces client code to specify a type parameter, which isn't a very friendly API.

In light of that information/warning, OP is asking if there would be any way of implementing their extension method so that there's no need for the caller to specify the generic type parameter, which would remove the compiler warning.

As for asking about a specific issue and whether that makes it off-topic, we've had "specific issues" posted here before, and we'll have more in the future - I deem it on-topic, and encourage reviewers to address any & all aspects of the code, even if it doesn't address the compiler warning specifically.

If someone comes along with a brilliant idea to overcome the C# generics limitation that doesn't allow this:

public static IList<SelectListItem> SelectListFor<T>() where T : Enum

Then OP will have their specific answer, and answers about every other thing we could see.

\$\endgroup\$
0
\$\begingroup\$

Going by the letter the question is ...

Off-topic,

Well.. here we go with our Help Center

  • Is code included directly in my question? (See Make sure you include your code in your question below.)
  • Am I an owner or maintainer of the code?
  • Is it actual code from a project rather than pseudo-code or example code?
  • Do I want the code to be good code? (i.e. not code-golfing, obfuscation, or similar)
  • To the best of my knowledge, does the code work as intended?
  • Do I want feedback about any or all facets of the code?

From what I see, the answer to all these questions but the last is Yes.

So if we go strictly by the help center, this question is off-topic

not because...

the code is broken, but because the question does not accept feedback about any or all facets of the code. It's a specific question which might be okay for Stack Overflow.

It's probably a little forced for there, since it contains real code and not a MCVE, but if we go by the letter it should be okay there.

\$\endgroup\$
3
  • \$\begingroup\$ I thought that the policy with questions that ask a specific question was that we would review them regardless and not adhere strictly to the OP's request, but that they were't actually off topic unless the OP entirely just cared about their issue and outright ignored everything else? \$\endgroup\$ Jan 13, 2016 at 15:04
  • \$\begingroup\$ This answer is from the perspective of going by help center rules by the letter, which is relativized by meta consensus for some cases, so basically, yes you're right, but the help center is saying something slightly different \$\endgroup\$
    – Vogel612
    Jan 13, 2016 at 15:05
  • \$\begingroup\$ I also disagree with this premise. Just because a statement asks for a review on a specific point does not indicate they aren't interested in other aspects. Though I believe that is a silly indicator altogether, it's not even relevant here. \$\endgroup\$ Jan 13, 2016 at 15:05
-2
\$\begingroup\$

Asking reviewers to fix a specific issue is off-topic by definition from the Help Center:

However, if your question is not about a particular piece of code and instead is a generally applicable question about…

  • Best practices in general (that is, it's okay to ask "Does this code follow common best practices?", but not "What is the best practice regarding X?")
  • Tools, improving, or conducting code reviews
  • How to add a feature to or solve a problem with your code
  • Trouble-shooting, debugging, or understanding code snippets
  • Higher-level architecture and design of software systems

then your question is off-topic for this site.

The author of the question you linked was specifically asking on how to solve the problem of triggering warning CA1004.

While the warning does not make the code broken, or uncompilable, it's still violating the rule asking to solve a problem with your code.


While this is the case for the question, editing the question to be a proper question about the code, and simply including at the end of the question:

Additionally, this code triggers warning CA1004, but it still compiles & works.

\$\endgroup\$
9
  • \$\begingroup\$ It depends a little on how you define "problem" here. I'd say that warnings are not problems per se and thus the reasoning here isn't quite fitting \$\endgroup\$
    – Vogel612
    Jan 13, 2016 at 15:01
  • 2
    \$\begingroup\$ The warning is a problem to the Author, and they want it fixed. That's how I'd define a problem. Read the last point of my review. \$\endgroup\$
    – Quill
    Jan 13, 2016 at 15:02
  • 5
    \$\begingroup\$ I disagree with the idea that a simple rephrasing suddenly makes the difference between off- and on-topic. It's entirely nitpicky that just because something is not phrased exactly how you like it, something should be closed. Code Review is by definition about problems with code but not the problems that are intended in the help center. \$\endgroup\$ Jan 13, 2016 at 15:03
  • 2
    \$\begingroup\$ It's not a simple rephrasing, it's rephrasing the entire question besides the code. \$\endgroup\$
    – Quill
    Jan 13, 2016 at 15:04
  • 2
    \$\begingroup\$ The question does not matter. People review any aspect they choose anyway. The question is entirely irrelevant in my eyes. If the requirement for a good question is mentioning that you're interested in all aspects reviewed then just PRETEND it is implicitly asked at every question and anything else is in addition to that, not as a replacement to it. \$\endgroup\$ Jan 13, 2016 at 15:08
  • \$\begingroup\$ @Quill Having too many if-statements is also a problem to many authors, and many Lint-tools will give warning messages for too many if-statements, too much cyclomatic complexity, etc. Did 50% of our questions just get off-topic? \$\endgroup\$ Jan 13, 2016 at 15:36
  • \$\begingroup\$ @SimonForsberg whether a lint-tool says that is irrelevant to the editor whose target's title says "halp - too many if statements" \$\endgroup\$
    – Quill
    Jan 13, 2016 at 15:37
  • \$\begingroup\$ @Quill You argued that the warning was a problem to the author and therefore it was off-topic. All warnings can be treated as problems, and considering all the compilers and tools that can cause warnings, if we treat warnings as off-topic then a lot of our existing on-topic questions are deemed off-topic. I don't see how your latest comment responds to this. \$\endgroup\$ Jan 13, 2016 at 15:43
  • 1
    \$\begingroup\$ @SimonForsberg I think there's a difference between using something the OP may have no clue about to deem if a question is on-topic, or if all the questions in the question are about a warning they want to fix. With a title about that warning too... \$\endgroup\$
    – Peilonrayz Mod
    Jan 13, 2016 at 15:57

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .