Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Remove contentious rule FC001 #86

Closed
jtimberman opened this Issue · 18 comments
@jtimberman

Originally the FC001 rule matched against symbols, which I brought up (issue #1) as contrary to what Opscode and other Chef trainers using Opscode's materials teach students, which is to access node attributes with strings as keys.

Read the original issue for more background on my reasoning. The rule itself is contentious, as people have varying preference between node attributes as a string, symbol or method. Symbols are the leading way to access attributes per the survey I posted (233 respondents in ~2 weeks), followed by strings. While this is not representative of every Chef user, the trend for these %'s was consistent while the survey was open.

This rule was brought up in assorted conversations at the Chef Community Summit last week, and I propose that the rule be removed from foodcritic entirely. Functionally there is absolutely no difference between symbols and strings, as symbols in node attributes are converted to strings by the underlying library anyway. This is purely a "what color should we paint the bikeshed?" issue.

Here's the pie graph of the survey's results:

https://www.evernote.com/shard/s5/sh/1fc5a0c9-bdd0-44f4-8f5a-ed2ddc9d2cfd/a13f36acd7cfa2a468f7829e5549209f

@jtimberman jtimberman referenced this issue from a commit in jtimberman/foodcritic
@jtimberman jtimberman Remove FC001 per #86 0a4c56d
@acrmp acrmp referenced this issue from a commit
@jtimberman jtimberman Remove FC001 per #86 8dedb46
@acrmp acrmp closed this
@greenmoss greenmoss referenced this issue in Youscribe/kvm-cookbook
Merged

Partial support for RHEL and CentOS #1

@acrmp acrmp referenced this issue from a commit
@acrmp Remove FC001 docs, refs #86. e8e81d7
@sethvargo

Just because people prefer symbols doesn't mean they are the best choice.

Functionally there is no difference, but pedagogically there is, thus I argue this is not a bike shed. Symbols introduce multiple edge cases, specifically interpolation, performance issues, and standardization.

Interpolation

You can't interpolate with symbols. So if you're dynamically combining a node attribute, you have to jump between symbols and strings - why not just use strings in the first place? This is incredibly confusing to a new Cheffer. "Use symbols, unless you need to interpolate or use a variable, then use strings". Why does that even make sense?

You're going to have to use strings at some point, so why not use them all the time.

Performance Issues

Under the hood, Chef converts everything to a mash, so it really doesn't matter how you define your attributes... unless you're running ree that is. On ree, converting a symbol of > length 32 leaks memory. This is a known and documented issue. So, for older organizations running ree, adopting Chef in their infrastructure may prove to be a HUGE memory hog because of the underlying mash conversion.

This is admittedly a stupid argument, but worth noting.

Standardization

This is the bulk of my argument. I really don't care how we lookup attributes, but we should use one. Do you know how confusing it is to a new Cheffer to grasp the idea that [:attribute], ['attribute'], and .attribute are all the same thing? The community cookbooks are littered with a wide variety. Pick a standard and stick to it. Strings are the only logical standard.

There's no reason that Chef should support 100 different ways to write/lookup attributes. From a pedagogical standpoint, it's an absolute nightmare.

Other Stuff

  1. From a maintenance standpoint, it makes finding attributes difficult in a large project. Do I search for [:attribute], ['attribute'], or .attribute. If I'm contributing to a community cookbook, I have to determine if the maintainer "chose" symbols, strings, or methods.

  2. If I'm search across a project for an attribute, I have to use a regex or do 3 searches. That's dumb.

  3. Both environments and roles use strings for the attributes. If you're using JSON (as you should be) for those, you have to use strings.

  4. I don't think the small sample at Summit is enough to remove this rule - they are intermediate-expert Chef users.

Solution

@acrmp I propose adding this rule back with it's own, dedicated tag (FC001) which people can choose to ignore.

To summarize, this was and still is a great rule that should not be removed. It reduces the complexity and ambiguity of attributes which helps with standardization. Please add it back.

@sethvargo sethvargo referenced this issue from a commit
@sethvargo sethvargo Revert "Remove FC001 per #86"
This reverts commit 8dedb46.
ca806e6
@Atalanta

Agree 100% with Seth. While I don't doubt for a fraction of a second that when writing high-performance pure Ruby code, there are performance and design/stability arguments in favor of symbols, they don't apply in Chef, and the inconsistency and management overhead caused by the TIMTOWDI approach to attributes is a strong enough reason to reinstate the rule. Seconded, wholeheartedly.

@mlafeldt

Remember that you can always re-add the rule using a custom rules.rb file if you want.

@sethvargo

I think it's a lot harder to go through and re-add the rule than just ignore it. It's a matter of style - that's the WHOLE point of a linter.

@mattscilipoti

I also feel pretty strongly that we should make a style choice and set that as the default. These recipes are meant to be shared. Consistency is very helpful. We should have to make a conscious choice to move away from that default. As others have said, it is quite easy, for those who do not agree, to exclude the rule.

My vote is for using a string as the default. I was moved by the "this can be the 'one' way" argument.

@erran-r7

I prefer Symbols as keys. They can use interpolation. I use it often actually.

1.9.3p392 :010 > :"foo#{5}"
 => :foobar
1.9.3p392 :011 > :"foo#{5}".class
 => Symbol

Why not use Symbols, the Ruby standard, when trying to create a standard for Chef attributes? It seems that the majority of people voted Symbols. Also performance always helps me decide between Strings and Symbols.

The pie chart shown in the issue:

@sethvargo

@ecarey-r7 I see four major issues. While I agree it works:

  1. You have introduced 3 ways to do the exact same thing:

    :foo
    :'foo'
    :"foo"
  2. Interpolation on symbols carries the same restriction as just using strings:

    :"foo#{'bar'}" #=> :foobar
    :'foo#{\'bar\'}' #=> :"foo\#{'bar'}" (notice this is a different type of symbol)
  3. You're losing performance. Under the hood, ruby is going to interpolate that string, and then call #to_sym on it. So you're note about performance is actually incorrect.

    That being said, is performance really an issue? Symbols are faster than strings because they aren't garbage collected. This is during a Chef run, not some long-running rails application. Garbage collection will happen automatically at the end of a Chef run when ruby terminates. Given that the longest Chef run I've ever seen took 45 minutes, I'm not so sure performance can be used as a valid indicator here.

  4. You're making me type one more character than if I just used a string in the first place...
@ghost Unknown referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@gmanfunky

Maybe it's trolly nature... but...

  1. "making me type one more character"...

And all y'all are arguing against the method crowd -- the group that can legitimately claim to save two or three characters per specifier (not including shift+) in the much more common use case of NOT wanting string interpolation.

::chuckle::

@sethvargo

I think @mattscilipoti is the only objective comment on this thread. This is about style and consistency, not choice or preference.

@juliandunn

I would like to again advocate putting the rule back in. Of all the cookbooks that I see, I would say that upwards of 95% of them are using strings as attribute keys anyways. And that's what we teach people in our training materials: to standardize on strings. All Chef Software, Inc.'s cookbooks use strings. (Except maybe some really ancient, legacy ones.)

Using symbols confuses people who aren't Rubyists and we need to make it easier for people to use Chef.

@acrmp
Owner

@juliandunn Hi Julian. FC001 was restored a few weeks ago (in master). See #97.

@juliandunn

Sweet, thanks @acrmp !

@ccope ccope referenced this issue in brightcove/cookbook-razor-server
Merged

Typo fix, syntax cleanup, better testing #7

@phromo

Just started out with chef, ran foodcritic on my new sweet cookbook, and got spammed with FC001.

Disappointed. I'm a rubyist and I'd go for symbols before strings any day, for reasons generally rationaled in the ruby community. I don't think enabling FC001 is a good default behaviour. If you want to enforce strings, do it in your own repo. I feel my confidence in foodcritic quickly declines when the first thing it does is tell me I'm wrong when I'm using an established ruby pattern...

Now I'll go figure out how I disable FC001 locally =)

@jrwesolo

@phromo You can disable specific rules like this:

foodcritic -t ~FC001 .
@greglu

To disable the rule from a Rakefile task:

FoodCritic::Rake::LintTask.new do |t|
  t.options = { tags: %w(~FC001) }
end
@AnalogJ

I just ran into this issue today, and after spending half a day trying to figure out a way to serialize and deserialize default[:options] = [:admin, :view, :create_job] and default[:role] = :support I've finally given in and converted all my attributes and values to strings.

This specifically cropped up when I tried to override the options attribute using an [environment].json file. Everything works perfect while your working in ruby, but once you attempt to create overrides that convert to json (eg. Environments and Roles) everything blows up. If you have conditional code that checks against a symbol, good luck.

@simonhaslam

I'm relatively new to Chef/Ruby, but one observation that hasn't been mentioned here is that if you're using an editor that recognises Ruby syntax, like Sublime Text, then symbols are highlighted differently to strings which I think is useful. You may even get code completion with symbols (probably based on those already present in the file). I realise that this arguably should be "fixed" in the tool, but may be a factor.

@lamont-granquist
Collaborator

Dunno if anyone still cares, but the Chef implementation of Mashes converts all symbols internally to strings anyway, so arguments about how ruby does things are irrelevant since symbols are always slower in Chef. Given that and the fact that new users will stumble over :foo_bar working vs. :foo-bar failing (and then needing to learn the :"foo-bar" syntax), the winner here is strings.

And it used to be doubly-awful to use symbols since not only would they get converted to strings, but the symbols that were created first would never be garbage collected until very recent MRI rubies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.