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

Consistify whitespace, code style in Ruby, CoffeeScript, SCSS #711

Merged
merged 9 commits into from
Apr 24, 2019

Conversation

jmromer
Copy link
Contributor

@jmromer jmromer commented Apr 23, 2019

  • Runs rufo on project ruby (NB: the .rufo config file is being deprecated)
  • Autoformats SCSS with prettier
  • Kills trailing whitespace in coffeescript with sed
  • Adds an .editorconfig to automatically consistify whitespace

@sethherr The diff is mostly quotes and whitespace changes, but if any of those commits seem like overkill or too much to pack into a single PR, I'm happy to remove them

Fixes #674

Jake Romer added 4 commits April 23, 2019 13:50
```
rufo .
```
```
stylesheets % prettier --write **/*.scss
```
```
sed --in-place 's/[[:space:]]\+$//' javascripts/**/*.coffee
```
@codeclimate
Copy link

codeclimate bot commented Apr 23, 2019

Code Climate has analyzed commit e5f9104 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 3

The test coverage on the diff in this pull request is 86.5% (80% is the threshold).

This pull request will bring the total coverage in the repository to 87.5% (0.0% change).

View more on Code Climate.

@jmromer jmromer marked this pull request as ready for review April 23, 2019 18:24
@jmromer jmromer requested a review from sethherr April 23, 2019 18:24
@jmromer jmromer self-assigned this Apr 23, 2019
Copy link
Member

@sethherr sethherr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! Exciting. This is great! I don't think this is too big, I think we should just get it done all at once, since it's going to cause merge conflicts no matter what. I do understand not dealing with JS in this PR though.

Could you add something to the readme explaining what is up with code formatting, or would you prefer to add that to a separate PR? Two specific topics (though I think more could be covered):

  • EditorConfig does look awesome. I just downloaded the sublime plugin. There should be some note of this, since I assume having it will now be expected?
  • We have a .eslintrc and a .rubocop.yaml - do these matter anymore?

@jmromer
Copy link
Contributor Author

jmromer commented Apr 23, 2019

  • We have a .eslintrc and a .rubocop.yaml - do these matter anymore?

I think .eslintrc is worth keeping for JS files, unless we want to add something like prettier-standard, which is effectively the most opinionated autoformatter for JS I know of.

Rubocop has checks that go beyond rufo's intended purpose (e.g., for Rails suggested conventions), so we may want to keep it around and continue using it as a linter. I can add some config rules to bring it in closer in line with rufo's settings. I think this would be as simple as disabling style cops:

https://github.com/ruby-formatter/rufo/blob/14802ef1881acf6ac116368ed357918cd1014262/.rubocop.yml#L7-L14

@jmromer
Copy link
Contributor Author

jmromer commented Apr 23, 2019

Could you add something to the readme explaining what is up with code formatting, or would you prefer to add that to a separate PR? Two specific topics (though I think more could be covered):

Will do! Forgot it requires some setup, thanks for the head's up

Jake Romer added 5 commits April 23, 2019 16:34
Performance cops are being moved to their own gem:

```
Performance Cops will be removed from RuboCop 0.68.
Use rubocop-performance gem instead.

Put this in your Gemfile.

  gem 'rubocop-performance'
```
@jmromer
Copy link
Contributor Author

jmromer commented Apr 23, 2019

@sethherr Rendered README update: https://github.com/bikeindex/bike_index/blob/e5f910439df169840f806db155ad338444c80ce2/README.markdown

NB: Looking a bit more closely, I see that prettier has Ruby support, which is interesting--potentially we could just use the one tool for JS, CoffeeScript (maybe ?), Sass, and Ruby. My impression is that it's faster than Rubocop but slower than Rufo. (We probably don't want to go too far down a rabbit hole evaluating formatting tools though.)

@sethherr
Copy link
Member

The idea of having a single formatter is definitely appealing - particularly if it means that all the style config would be in one file (which it looks like it would be?)

It also seems like prettier/plugin-ruby is under more active development than rufo and has more stars.

So... give it a try if you want to?

@jmromer jmromer requested a review from sethherr April 23, 2019 21:33
Copy link
Member

@sethherr sethherr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this works!

I'm happy to merge this, thanks for adding the notes in the readme.

I'm also happy if you want to check out prettier plugin-ruby.

@jmromer
Copy link
Contributor Author

jmromer commented Apr 23, 2019

So... give it a try if you want to?

No prob! I'll take a look.

Update: I like the idea of using a single formatter for everything, but prettier is way too aggressive with Ruby and introduces a ton of breakages that I don't think are worth spending time addressing. I'll move forward with rufo for now.

@jmromer jmromer force-pushed the add-rufo branch 4 times, most recently from a519858 to e5f9104 Compare April 24, 2019 00:29
@sethherr
Copy link
Member

Ok. Is this ready to merge @jmromer ?

@jmromer
Copy link
Contributor Author

jmromer commented Apr 24, 2019

@sethherr All set! 👍

@sethherr sethherr merged commit d171f69 into master Apr 24, 2019
@sethherr sethherr deleted the add-rufo branch April 24, 2019 14:51
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

Successfully merging this pull request may close these issues.

Add Rufo for ruby formatting
2 participants