Add new Lint/BooleanSymbol cop

[droptheplot]

A few days ago I had this bug when someone accidentally used :true instead of true. This leads to some problems since both :true and :false are "truthy" values and it's hard to notice this because they are quite normal symbols.

I think in most cases you want to use just standard booleans so I suggest to mark it as an issue.

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Used the same coding conventions as the rest of the project.
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• All tests(rake spec) are passing.
• The new code doesn't generate RuboCop offenses that are checked by rake internal_investigation.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.
• Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

[Drenmi]

I think the cop description and the message should allude to the fact that this might be accidental, since there are actual cases where you'd want :true or :false, and just saying "use true instead" is actually wrong. WDYT? 🙂

[droptheplot]

Yes, you're right. I made message similar to Lint/AssignmentInCondition.

[Drenmi]

Interesting case! 🙂 I think this can be enabled by default, but could actually be turned off in RuboCop's own .rubocop.yml, since the codebase deals heavily with describing code, and legitimate uses of :true and :false will be more common than accidents. WDYT?

[droptheplot]

@Drenmi That's right, I disabled it for internal_investigation.

[bbatsov]

This empty line seems redundant to me.

[bbatsov]

Same here.

[bbatsov]

Symbol with a boolean value name (or something like this). "Boolean symbol" sounds a bit strange to me.

[bbatsov]

That seems like an overkill - you can just check if the symbol is :true or :false.

[bbatsov]

I'd expand this text a bit.

[droptheplot]

@bbatsov, fixed.

[rrosenblum]

My first thought when reading through the description was cool, should this also look for 'true' and 'false'. Then I started thinking about the use cases for that which wouldn't always be safe to convert the string versions because of cases like environment variables. Then I started questioning the usage for symbols.

Is this going to be prone to a high volume of false positive?

[rrosenblum]

While we are at it, we might as well add in the bad and good example for false. 2 extra lines of documentation that would then cover 100% of the cases.

[rrosenblum]

Any thoughts on using a node matcher?

def_node_matcher :boolean_symbol?, '(sym {:true :false})'

def on_sym(node)
  return unless boolean_symbol?(node)
  add_offense(node, :expression, format(MSG, node.source[1..-1]))
end

[rrosenblum]

Would it provide value to add a test for the boolean versions of true and false?

[droptheplot]

@rrosenblum Thanks, I've made changes according to your comments.

Is this going to be prone to a high volume of false positive?

For example I checked rails:

> ack -hc "true"
6398
> ack -hc "'true'"
39
> ack -hc " :true"
4

And rubocop:

> ack -hc "true"
2573
> ack -hc "'true'"
11
> ack -hc " :true"
3

I think it should be treated like Lint/AssignmentInCondition.

[droptheplot]

@bbatsov, thanks!