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

Enforce / allow styles #11

Closed
meaku opened this issue Mar 13, 2017 · 7 comments
Closed

Enforce / allow styles #11

meaku opened this issue Mar 13, 2017 · 7 comments

Comments

@meaku
Copy link
Member

meaku commented Mar 13, 2017

I had a discussion with @jhnns on Saturday about coding rules in general. I totally agree that some rules with a big impact have to be enforced. Things like single quotes vs. double quotes and literal spacing is something we want to keep consistent.

Other rules like dangling commas #10 could be something we allow, but don't enforce.

What do you think?

@acidicX
Copy link
Contributor

acidicX commented Mar 13, 2017

single quotes vs. double quotes

doube quotes are TW btw. You can always override rules in your setup anyway, if you disagree with them 😉

The pg eslint config is not the bible. So I would say enforce the rules in this config, override in your project if you/your team disagrees.

@jhnns
Copy link
Member

jhnns commented Mar 13, 2017

doube quotes are TW btw

Why?

You can always override rules in your setup anyway, if you disagree with them

Yes, but I'd say you should have a good cause.

The pg eslint config is not the bible. So I would say enforce the rules in this config, override in your project if you/your team disagrees.

No, but they don't make sense if every projects overrides a lot of rules. If there are problematic rules, we can discuss them.

@meaku
Copy link
Member Author

meaku commented Mar 13, 2017

You can always override rules in your setup anyway, if you disagree with them

The main purpose of our eslint config is to have a unified style. We can't all agree on everything and there will always be someone unhappy. But if we have those rules, they should be applied on every project and only be altered on the project level for good reason. Personal taste is not a reason to alter the config.

@acidicX
Copy link
Contributor

acidicX commented Mar 13, 2017

No, but they don't make sense if every projects overrides a lot of rules

True, but we are talking about one rule here. Also, the effect of "we allow, but don't enforce" is the same - you can choose what you want by overriding or by leaving out the rules.

Personal taste is not a reason to alter the config.

single vs double qoutes are merely personal taste for example, afaik there are no downsides to using one or the other except from escaping strings (depending on which language you are using). Why not use what your team likes more. I have no problem to adapt, and a little bit of freedom keeps everyone happy.

@flootr
Copy link

flootr commented Mar 13, 2017

Maybe we should differentiate between Open Source modules and our own projects vs. (private) client projects. The latter is open for some customizations imho. It may be sensible to adjust some rules there depending on the project.

@jhnns
Copy link
Member

jhnns commented Mar 14, 2017

We can't all agree on everything and there will always be someone unhappy

Yes, but I would like that all of you are as happy as possible 😁. I know that it's annoying to write code with a code style that is in your way all the time. I'm always open to suggestions and changes, this should be our code style, not mine. And we have to re-validate the rules all the time because as new language features arrive, there might be better ways to do things (like const/let over var). It might also turn out that our personal preferences have changed.

I'm thinking about making this as objective as possible because we won't get far with things like "I think it's ugly" or "I don't like it". If you don't like it, there is usually a deeper cause than that – and you should find it. If the true cause is "It's unfamiliar", than you should give it a try. The current code style is the result of years working on different code bases and balancing developer ergonomics against "good" code, which is code that is readable, changeable and less error-prone.

What about this: If somebody wants to change a rule, we should collect arguments for both sides. On the left side, there should be the status quo, on the right side the new rule. Status quo should always have the argument that "It's the status quo" because changing code manually is cumbersome. However, if a rule is fixable via --fix, this is automatically an argument for the new rule because the change is not involved with a lot of work. After collecting all the arguments, every team member can weigh arguments. This way, it makes clear that it's about weighing arguments than instead of "I like it/I don't like it". You also have to vote for counter-arguments, which is reasonable because if the argument is accepted, it should be a valid point.

Example Dangling Commas:

Status Quo Dangling Commas
L1 Status Quo R1 Fixable
L2 More familiar because lists in common languages don't end on commas R2 Moving items becomes easier
L3 Unfamiliar developers may expect that array literals with dangling commas have one undefined item at the end R3 Cleaner diffs
  R4 Valid ES3 syntax

Personally, I'd weigh the arguments this way:

  1. R2 (highest)
  2. R3
  3. L2
  4. R4
  5. L3
  6. R1
  7. L1 (lowest)

Someone else may weigh it this way:

  1. R3 (highest)
  2. L2
  3. R2
  4. L1
  5. L3
  6. R4
  7. R1 (lowest)

After collecting all votes, we create a common ranking and we'll see which rule has more points:

Rule Points
L1  5
L2 11
L3 6
R1 3
R2 12
R3 13
R4 6

Dangling Commas (R) won with 34 against 22. This might seem complicated on first sight, but is easily automatable and as a result, we will have better discussions. The interesting thing with this voting system is that rules could win even if they have less arguments. That's ok because maybe people think that these arguments are more important than the other ones (this addresses my general concern with pro/contra lists 😁)

@jhnns
Copy link
Member

jhnns commented Mar 31, 2018

Follow up of the discussion #24

@jhnns jhnns closed this as completed Mar 31, 2018
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

No branches or pull requests

4 participants