Confusion with symbols and strings

[troessner]

Right now we're mixing them everywhere unfortunately which is confusing and error-prone.

Suggestion:

• we transform every key coming in via config to symbols
• we transform all internal constants from string to symbol. E.g. "accept" -> :accept
• all hash keys are expected to be symbols

WDYT?

[mvz]

I agree in principle. Since I'm going to go over part of the config, how about I pick some of that up as well? That way we can avoid another big bang change.

I would also like to get rid of constants used just for config keys. I much prefer using the literal symbol instead (not so much literal strings, though).

Hm, I'm thinking if we force keys to symbols from the config, we will have to update the keys in the smell classes everywhere as well. So, we will have to do this all in one go anyway.

[troessner]

Since I'm going to go over part of the config

What part are you referring to?

I would also like to get rid of constants used just for config keys. I much prefer using the literal symbol instead (not so much literal strings, though).

Yes, please.

Hm, I'm thinking if we force keys to symbols from the config, we will have to update the keys in the smell classes everywhere as well. So, we will have to do this all in one go anyway.

Yep, this will be a bigger change for sure.

But after that we can finally rely on symbols which will increase my productivity about 2000% ;)

[mvz]

What part are you referring to?

#694.

[troessner]

Ah, ok, makes sense ;)

[chastell]

I’m all for symbols as keys everywhere, provided we keep the string keys in read/written YAML files (we have to do it for backwards compatibility anyway). Symbol keys in YAML files are way less nice than string keys, but I’m for turning them to symbols upon reading.

I don’t think we should care about symbols not being gc’d in Ruby < 2.2, as I don’t see any reasonable vectors of attack (a malicious reek config taking down a bigger tool using reek…?).

[mvz]

provided we keep the string keys in read/written YAML files

I always assumed that was the idea, yes.

[troessner]

I always assumed that was the idea, yes.

It was! ;)

[troessner]

I'll look into this since I believe it is vital for Reeks future to get this right.

[troessner]

@mvz still assigned to you - do you think you'll take care of this any time soon? If not, I'd volunteer to take over.

[mvz]

@troessner feel free to take over.

[troessner]

I'd like to back-pedal on this one big time. While waiting for the as-usual-fucking-late-flight I started looking into this and this would be a huge refactoring that would touch literally half of Reeks source files and tests.
Now that would still be ok but I have come to the conclusion that it is not worth because I don't think it will improve clarity. If anything, it will make things worse. I'm 10% in and it's already a bloody mess.

How about approaching this differently. What we are striving for is not necessarily that all-the-things are symbols but consistency.

How about this simple rule of thumb:

Everything that is coming from "outside" is a string and will be treated as string everywhere.
Meaning:

• cli arguments
• config file
• code comments

If we agree on this rule the current set up is consistent as it is right now.

WDYT?

[mvz]

The one thing I would really like to get rid of is all the MAX_BLAH_KEY = 'max_blah' stuff we have lying around. However, this may be an orthogonal problem that can be fixed without symbolizing all the things.

[mvz]

On not making a change that hits every file: 👍.

[Drenmi]

Wild TEAM LEAD appeared.

Everything that is coming from "outside" is a string and will be treated as string everywhere.

The technical merit for this is sound. Downside is it resists programmatic enforcement--and education is at least one order of magnitude more expensive than programmatic enforcement.

That said, if this has worked reasonably well so far, I am sure it is a price that can be paid. 😊

[mvz]

@Drenmi we're basically only talking about hash keys here, and the choice is limited to strings or symbols. Not much enforcement either way.

There are configuration objects, but that's another story.

[troessner]

The technical merit for this is sound. Downside is it resists programmatic enforcement--and education is at least one order of magnitude more expensive than programmatic enforcement.

Fully agree with that in general. Big factor is with how many conventions like this you have to juggle at the same time. I think this is still manageable.

[chastell]

My æsthetic is to go with symbol keys, but I agree it’s not worth doing at the moment, especially when we haven’t had an actual issue with this so far – so +1 for sticking to the current situation and reopening if needed.

[troessner]

Ok, seems like we have an agreement to leave it like it is, closing this one.