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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spike prettier rubygem #116

Closed
wants to merge 2 commits into from

Conversation

AlanFoster
Copy link
Contributor

@AlanFoster AlanFoster commented Feb 17, 2019

The theory is Ruby developers don't want to deal with setting up npm/node locally, or on CI servers.
Instead we'd provide a prettier gem, and we'll use https://github.com/zeit/pkg to create a standalone binary for prettier - and embed the javascript src files for plugin-ruby - as well as any additional rubocop configuration files.

I've tested this as a proof of concept with:

yarn build:gem

Then I created a new rails project and added the Gem for testing purposes:

gem 'prettier', group: :development, path: '../prettier-ruby/gem'

From the new rails project:

$ bundle exec ruby-prettier --write
app/channels/application_cable/channel.rb 219ms
app/channels/application_cable/connection.rb 202ms
app/controllers/application_controller.rb 200ms
app/helpers/application_helper.rb 202ms
app/jobs/application_job.rb 196ms
app/mailers/application_mailer.rb 197ms
app/models/application_record.rb 206ms
config/application.rb 196ms
config/boot.rb 196ms
config/environment.rb 201ms
config/environments/development.rb 203ms
config/environments/production.rb 205ms
config/environments/test.rb 205ms
config/initializers/application_controller_renderer.rb 196ms
config/initializers/assets.rb 195ms
config/initializers/backtrace_silencers.rb 195ms
config/initializers/content_security_policy.rb 196ms
config/initializers/cookies_serializer.rb 202ms
config/initializers/filter_parameter_logging.rb 203ms
config/initializers/inflections.rb 195ms
config/initializers/mime_types.rb 195ms
config/initializers/wrap_parameters.rb 193ms
config/puma.rb 196ms
config/routes.rb 202ms
config/spring.rb 198ms
db/seeds.rb 198ms
test/application_system_test_case.rb 200ms
test/test_helper.rb 201ms

This is just a spike - I'll clean this PR up as appropriate. Let me know your thoughts @kddeisz 馃憤

#!/usr/bin/env ruby

def prettier
target = if RUBY_PLATFORM =~ /darwin/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pkg by default supports macos, linux, windows. But supports more

https://github.com/zeit/pkg/tree/4bdbd6a95f64479a3e651a155fa0d72f1f38c489#targets

Copy link
Member

Choose a reason for hiding this comment

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

The defaults you chose make a lot of sense. Let's go with those until we get a report for other support.

package.json Outdated
@@ -5,6 +5,7 @@
"main": "src/ruby.js",
"scripts": {
"test": "jest",
"build:gem": "pkg --output gem/pkg/prettier --targets=linux,macos,win node_modules/prettier/bin-prettier.js && cp -r src gem && cp package.json gem",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to put the gem files into a separate folder for simplicity's sake, but all of the gem related files could be at the top level so there's no need to copy across the src directory or package file.

@kddnewton
Copy link
Member

Wow this is so great. Okay a couple of things I think we should do before shipping this:

  • Let's go with making this a top level thing. I think it'll be easier to understand if the gemspec is at the top level.
  • Sync up the version of the gem with the package. Maybe by parsing the package.json? That way we don't have to update it in multiple places.
  • Make pkg a development dependency, as we don't want to force that on end users.
  • Make a rake task that people can inherit and use. I like reek a lot and they have good docs on this: https://github.com/troessner/reek/blob/master/docs/Rake-Task.md and https://github.com/troessner/reek/blob/master/lib/reek/rake/task.rb should get you going.
  • Don't force the user into '**/*.{rb,rake}', instead just pass that in as a command line argument. As in prettier-ruby path/to/file.rb. Also you shouldn't need to specify the parser.

This is so great Alan! Thanks for the work.

@kddnewton
Copy link
Member

One more thing - we should ship a rubocop config with this gem so that folks can use inherit_gem and just take our rules. In general I think this would just be running down the list of cops and disabling everything that this gem handles. This would close #55.

@AlanFoster AlanFoster force-pushed the spike-prettier-rubygem branch 2 times, most recently from 2b868cd to 9888db2 Compare February 17, 2019 23:55
prettier.gemspec Outdated Show resolved Hide resolved
src/ripper.rb Outdated
build_sexp(:bodystmt, body).tap do |sexp|
attach_comments_to(sexp, body[0])
end
build_sexp(:bodystmt, body).tap { |sexp| attach_comments_to(sexp, body[0]) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate PR for formatting this file: #123

@AlanFoster AlanFoster force-pushed the spike-prettier-rubygem branch 2 times, most recently from 510e85d to a3ab6fa Compare February 18, 2019 00:20
@AlanFoster
Copy link
Contributor Author

AlanFoster commented Feb 18, 2019

Let's go with making this a top level thing. I think it'll be easier to understand if the gemspec is at the top level.

Done 馃憤

Sync up the version of the gem with the package. Maybe by parsing the package.json? That way we don't have to update it in multiple places.

Done 馃憤 I'll spend some time tomorrow grabbing some extra meta-data from this file too, like the author, homepage, etc.

Make pkg a development dependency, as we don't want to force that on end users.

Done 馃憤

Make a rake task that people can inherit and use. I like reek a lot and they have good docs on this: https://github.com/troessner/reek/blob/master/docs/Rake-Task.md and https://github.com/troessner/reek/blob/master/lib/reek/rake/task.rb should get you going.

I'll punt on this to a different PR - as this PR will hopefully just be a good first step towards getting a standalone gem 馃憤

Don't force the user into '**/*.{rb,rake}', instead just pass that in as a command line argument. As in prettier-ruby path/to/file.rb. Also you shouldn't need to specify the parser.

Done 馃憤 Everything should align with Prettier's semantics, the Thor CLI is pretty much just passing all the arguments straight through now.

One more thing - we should ship a rubocop config with this gem so that folks can use inherit_gem and just take our rules.

Done 馃憤 I'm shipping the current .rubocop.yml file with the gem now; I've updated the read me now too with an example of usage:

inherit_gem:
     prettier: .rubocop.yml

In general I think this would just be running down the list of cops and disabling everything that this gem handles. This would close #55.

I'm including the current .rubocop.yml file for now, and I'll put up a separate PR for figuring out the specific rules we need to disable.

I'm hoping that I've addressed most of the things now; Let me know your latest thoughts @kddeisz 馃帀

@AlanFoster
Copy link
Contributor Author

Just double checked that this approach actually works as expected, and all seems well on a fresh docker container which only has ruby:

$ docker run  -v `pwd`:`pwd` -w `pwd` -it ruby:2.6.1-stretch /bin/bash
$ gem install bundler
$ bundle
$ bundle exec ruby-prettier --version
0.6.2
$ bundle exec ruby-prettier --write src/ripper.rb
src/ripper.rb 300ms
$ node --version
bash: node: command not found

@AlanFoster AlanFoster force-pushed the spike-prettier-rubygem branch 2 times, most recently from d27d490 to 57fffbd Compare February 18, 2019 17:39
@orta
Copy link

orta commented Feb 20, 2019

nice work 馃憤

@AlanFoster AlanFoster force-pushed the spike-prettier-rubygem branch 2 times, most recently from 2ec8574 to 5b93413 Compare February 21, 2019 17:00
@hawkins
Copy link

hawkins commented Feb 22, 2019

This is super lovely, I'm definitely looking forward to getting my hands on this! Great work and thank you for it! 馃檶

@AlanFoster
Copy link
Contributor Author

AlanFoster commented Feb 24, 2019

@kddeisz Let me know your latest thoughts so I can apply the feedback and we can get this shipped - or if you're working on an alternative approach just let me know 馃憤

@kddnewton
Copy link
Member

@AlanFoster This is looking great! I want some time to sit down with it, and just haven't gotten the chance yet. No need to change anything, I'll take a look at it this week.

@orta
Copy link

orta commented Feb 26, 2019

Gave this a run with @yuki24 - worked well!

I don't think it picks up a prettierrc etc, so we couldn't change settings when using it via bundle exec but it felt good 馃憤

@yuki24
Copy link

yuki24 commented Feb 26, 2019

For those of you who want to give this a try without Docker or git pull, here are the steps I followed to build a package locally:

# Add this to the Gemfile
gem 'prettier', git: 'https://github.com/AlanFoster/prettier-ruby-1', branch: 'spike-prettier-rubygem'
$ bundle
$ cd `bundle show prettier`
$ yarn && yarn build:gem
$ cd -
$ bundle exec ruby-prettier --write path/to/file.rb

@AlanFoster
Copy link
Contributor Author

I don't think it picks up a prettierrc etc, so we couldn't change settings when using it via bundle exec

@orta Prettier is meant to be opinionated right? :trollface:

Good catch though! I'll look to see if I can fix this scenario as part of a separate PR - or this one if it feels like a blocker 馃憤

@orta
Copy link

orta commented Feb 27, 2019

Doesn't feel like a blocker to me, the hard part (the deployment etc) is the focus on this PR

@kddnewton
Copy link
Member

Okay so a quick update on the state of things -

  • Y'all are amazing and I really appreciate all of your contributions.
  • I really want the gem to be the de facto way to integrate prettier-ruby into apps, so I want it to come out swinging. For that reason I want to wait to push it out until we get some of the comment issues fixed (almost all of the open issues have to do with comments).
  • With my work, I don't have a ton of time to work on this during the work week. I'll comb through it in the morning, but for the most part most of my work gets done on this during the weekends. I'll take a look this weekend and try to churn through a couple of these things.
  • I'm working on a much bigger refactor of comments that I'm hoping to get out this weekend.
  • Keep the PRs coming! It's great to see so much activity. I'll get through them as quickly as I can.

Thanks again!

@AlanFoster AlanFoster force-pushed the spike-prettier-rubygem branch 2 times, most recently from 3de7c79 to 516c184 Compare March 1, 2019 12:10
@kddnewton
Copy link
Member

Thank you @AlanFoster! I've copied most of the changes in here into #193. Not going to merge the README changes until we fully release the gem. This is great!

@kddnewton kddnewton closed this Mar 3, 2019
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.

None yet

5 participants