Coding style (enforcement)

[quinn-dev]

The coding style you use is pretty unique and at times quite difficult to wrap my head around. At the same time, it's difficult to adopt your particular coding style in my PRs if I don't know if certain things are just formatting errors or deliberate design decisions (as evidenced by this PR).

To solve these issues, we could introduce either a code style or code format enforcement system.

A code style enforcement system would not just ensure proper formatting, but would enforce a set of best practices to improve the overall code quality. The options range from highly opinionated (standard, xo, airbnb) to highly configurable (ESLint).

A code formatter wouldn't meddle with the overall coding style but simply enforce a certain formatting style, i.e. trailing semicolons, indentation, trailing colons, etc. prettier works very well for this purpose.

As I can see, sleepdiary/dashboard already uses ESLint, but the current rules are insufficient to enforce either a cohesive coding style or coherent code formatting.

My personal preference would be a code style enforcement system like xo, since it yields high-quality code, discourages bad practices and ensures consistency. But in the end you (@andrew-sayers) are currently the main developer on this project so the system shouldn't be a hindrance in your development process.

[andrew-sayers]

This is a big topic, so I'll start with my few strong, general opinions that are relevant here:

Firstly, consistency is king. If two things look different, there should be a reason for it. To be blunt, if a patch looks the same as the lines above and below it, I rarely question it beyond that. I think you got unlucky hitting on the issue with trailing commas so quickly - there's no way you could have known how the projects I've worked on have made me unusually focussed on that problem.

Secondly, a king is not a tyrant. I cordially dislike most enforcement mechanisms, because they won't shut up about that one edge case where the "bad" thing is actually the right solution. For example, notice all the errors I've had to convert to warnings in the dashboard's .eslintrc in order to make the debugging process tolerable. Conversely, the library's entrypoint.sh is a good example of my preferred approach to enforcement - it only runs when you've got bandwidth to deal with problems, only cares about new issues since your last git push, makes sure you know what it wants you to do, then leaves you to get on with it. Actually to be more precise, right now it should refuse to automatically merge into the built branch, which may or may not turn out to be a good idea.

Finally, it's easier to ask for forgiveness than permission. Sometimes the only way to know "the" answer is to throw out "an" answer and see if anyone complains - for example, you've (reasonably) complained about a lot of the bad decisions I've made in this project, but it's been much easier to fix the minority of mistakes now than it would have been to wade through all the minutia as it came up. Here's another way to think about it: the whole of modern software development is about making it easy to try stuff and roll back if it causes problems, so you might as well embrace it!

In terms of automated solutions for checking code, my focus is currently on scanning for security issues. This link for one of your projects should take you to a page with a bewildering away of products that claim to find security problems. You can get there by going to a project you own, then Settings > Security & analysis > Code scanning > Set up. I'm afraid the sheer number of choices has bumped all automated code-scanning down my priority list, to somewhere after the pipelines have matured. If you're expecting me to implement something, it'll probably be long after that.

Given all of the above, how about this for a plan...

Pick a program you like - you've mentioned xo, so let's stick with that for now. Fork the info repository (which is small and generally low-risk), run xo and see what appears in git diff. If you notice a pattern of code that looks like bugs, git add -p those lines and make a PR we can discuss. If it complains about a pattern of code that looks deliberate, configure xo to ignore it then git reset --hard HEAD and start again. Eventually you should have a config file we can use as the basis of an automated test. Or failing that, you'll have a cordial dislike of an enforcement mechanism ;)

[andrew-sayers]

This is now part of the discussion in #4 - please consider discussing the issue there instead.