speckleworks/SpeckleServer

Formatting, linting, code style?

Closed this issue · 13 comments

logan12358 commented

Step 0:

Stemming from discussion on Slack, I've started working on the branch logan12358/formatting. So far I've added a .editorconfig and formatted all the files according to it, and added a (lenient; minimal changes required) ESLint configuration.

This issue is for continued discussion about formatting, and to track progress. Some things I'd like to hear people's thoughts on:

  • The proposed .editorconfig
  • Tools to use for linting/prettifying, and preferences around them

Here are the rough steps I see this issue tracking:

  • Set up .editorconfig
  • Set up very lenient ESLint rules
  • Adopt Standard, with the space-in-parens exception
  • Maybe consider adding further rules?
  • Write a pre-commit checklist somewhere
  • Set up CI for linting?
  • Put a note in CONTRIBUTING.md just saying that ESLint is set up
logan12358 commented

Tools-wise, I've started down the ESLint path because it seems big, flexible, and well-supported. If you have other thoughts or preferences though, let me know - this isn't something I'm an expert on!

radumg commented

Could we please also add a task/steps to update any relevant documentation, such README.md or CONTRIBUTING.md with any required dependencies, instructions or additional steps for users, etc ?

didimitrie commented

think it's mostly up to the devs to set up their editors for eslint... and the future ci will theoretically buzz when you're doing something wrong. the end users don't care (or they should, they'll get cleaner code!).

Looks good so far to me. Not an expert either, so standard disclaimers apply. We'll fumble along and see where we get (planning to pull this eve and see where i'm getting at). Then once @pearswj is back he's going to give us a talking to...

  • 😄1
mswaidan commented
logan12358 commented

@radumg That's a good point, there should probably at least be something in CONTRIBUTING.md to let people to know about linting that might make CI beep

logan12358 commented

@mswaidan It looks like eslint-plugin-vue might handle all the Vue stuff nicely. That would also apply to SpeckleAdmin.

logan12358 commented

I was just thinking about this issue. The current approach of adding in rules until we're happy seems kinda arbitrary, and seems like it will just result in rediscovering many best practices.

What would thoughts be on aiming towards an existing well-known style, such as Airbnb's or Standard or others?

The space in parentheses thing is probably the most noticable aesthetic difference, and it could potentially be overridden.

didimitrie commented

my only sticking points:

  • no semicolons (so standard seems like a better fit from this pow)
  • the spaces "space-in-parens": [2, "always"]

otherwise 👍

mswaidan commented
logan12358 commented

I just had a go at making everything follow Standard, and it was pretty painless (--fix got most of the way there). On the spaces, either:

  • We use the Standard ESLint rules but override space-in-parens. This is probably fine, but is slightly more complex to configure and less "standard".
  • We fully commit to Standard in the repo. Spaces in parenthesis is automatically fixable, so people wanting spaces could set up Git smudge and clean (I just tried and it's... functional) or set up some editor config to automatically translate.

What are your thoughts on those options @didimitrie and others? I'm kinda happy with either

logan12358 commented

Fair enough :) Check out cbd628d for the latest: Standard with space-in-parens overridden.