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

'exec' and 'restart' commands should automatically start the server instead of crashing #3

Merged
merged 17 commits into from
Dec 16, 2018

Conversation

ndbroadbent
Copy link
Contributor

@ndbroadbent ndbroadbent commented Dec 15, 2018

I think it's more common to do it this way, and it makes it more user-friendly. It's much better when you don't have to remember to start the server manually, because this makes it easier to integrate rubocop-daemon with a text editor (e.g. VS Code) or command-line shortcuts / automated linting.

Fixes #2

@ndbroadbent
Copy link
Contributor Author

Sorry I'm just trying to figure out how to make the rubocop-wrapper script work without the Ruby wrapper: rubygems/rubygems#88 (comment)

rubygems wraps any "executables" with a Ruby script, and this breaks the bash script.

@ndbroadbent ndbroadbent force-pushed the autostart-server branch 4 times, most recently from ba31d46 to 58300e9 Compare December 15, 2018 08:59
@fohte
Copy link
Owner

fohte commented Dec 15, 2018

Very thanks for your working! These features are so great!
I'm checking and testing now them.

rubygems wraps any "executables" with a Ruby script, and this breaks the bash script.

Yeah, as you already know, rubygems doesn't allow sh/bash script as executables unfortunately.
So we cannot automate installation of the rubocop-wrapper script, we should guide how to install it to users.

@ndbroadbent
Copy link
Contributor Author

Yes, I just updated the README with some manual installation instructions. And it works amazingly well!

I even added a symlink to /usr/local/bin/rubocop, and set ruby.rubocop.executePath to /usr/local/bin in VS Code. And now formatOnSave is so much faster, and I don't have to wait 4-5 seconds every time I save a file. This is really awesome!

bin/rubocop-daemon-wrapper Outdated Show resolved Hide resolved
@ndbroadbent ndbroadbent force-pushed the autostart-server branch 4 times, most recently from 19facd6 to ae77d22 Compare December 15, 2018 11:05
@ndbroadbent
Copy link
Contributor Author

There's one more thing I want to fix: I noticed that VS Code will run rubocop from the same directory as the current file, so this caused rubocop-daemon to start a lot of different daemons for each directory:

$ ls -l ~/.cache/rubocop-daemon/

Users+ndbroadbent+code+my_app
Users+ndbroadbent+code+my_app+app+jobs
Users+ndbroadbent+code+my_app+app+models
Users+ndbroadbent+code+my_app+app+serializers
Users+ndbroadbent+code+my_app+app+services
Users+ndbroadbent+code+my_app+spec
Users+ndbroadbent+code+my_app+spec+integration
Users+ndbroadbent+code+my_app+spec+models+concerns
Users+ndbroadbent+code+my_app+spec+support+helpers

I think it would make sense to search through the parent directories until it finds a Gemfile (or gems.rb) in one of the parent directories. This means that you could also start the server from a subdirectory, and re-use it in other directories.

I think looking for a Gemfile or gems.rb would be a good heuristic. I think almost every project that uses RuboCop will also have a Gemfile. And if not, they'll need to add one in order to use rubocop-daemon.

@ndbroadbent ndbroadbent force-pushed the autostart-server branch 2 times, most recently from b7db58b to 3b93790 Compare December 15, 2018 11:51
@ndbroadbent ndbroadbent force-pushed the autostart-server branch 2 times, most recently from af0f904 to 56ac0cb Compare December 15, 2018 12:00
@ndbroadbent
Copy link
Contributor Author

OK, I've finished all these changes, and now VS Code is using a single rubocop daemon for every file in my project.

I think that's everything I wanted to add, and it's working great for me.

…lt colorized output. (Can be turned off with --no-color.)
@ndbroadbent
Copy link
Contributor Author

I just fixed one more thing - The RuboCop output is colorized by default, but the color was turned off when using rubocop-daemon (or rubocop-daemon-wrapper with netcat.)
This is because rubocop uses the rainbow library:

It's enabled by default, unless STDOUT/STDERR is not a TTY or a terminal is dumb.

So to preserve this default behavior, I'm prepending the --color flag to the @args array. This preserves the default RuboCop behavior of printing colorized output. The colors can still be turned off by passing the --no-color flag. I also checked that adding this flag doesn't break any of the other flags (e.g. you can still run rubocop --color --help or rubocop --color --version, etc.)

…ing goes wrong with the nc command, try killing all the rubocop-daemon processes and clearing the cache folder before retrying
@ndbroadbent
Copy link
Contributor Author

ndbroadbent commented Dec 15, 2018

Another fix: I'm now returning the correct exit code from rubocop-daemon exec and rubocop-daemon-wrapper. This is important for git pre-commit hooks that rely on the exit status to know when there are any linting errors.

e.g.

$ rubocop-daemon exec -- spec/spec_helper.rb && echo SUCCESS

Before this change, rubocop-daemon exec would always exit with 0, so the echo command would always be run (even if there were errors.)

…and use this as the exit code for the client
@ndbroadbent
Copy link
Contributor Author

I've also needed to add a file lock (348fdce) to prevent multiple rubocop-daemon processes.

When you change a file in VS Code, it runs RuboCop linting rules. If you save the file at the same time, it will also run the Layout rules. So this starts two rubocop-daemon processes at the same time:

$ ps ax | grep rubocop-daemon
64325   ??  S      0:00.01 /bin/bash /usr/local/bin/rubocop -a -f simple -c /Users/ndbroadbent/code/rubocop-daemon/.rubocop.yml /var/folders/9t/hpqz7_5s27dc8_j_2hd7p_7h0000gn/T/rubocop8cZ8JD/tmp.rb
64332   ??  S      0:00.01 /bin/bash /usr/local/bin/rubocop -s /Users/ndbroadbent/code/rubocop-daemon/lib/rubocop/daemon.rb -f json
64344   ??  R      0:00.19 ruby /Users/ndbroadbent/.rvm/gems/ruby-2.5.3/bin/rubocop-daemon start

The last one to start will override the pid file, and then there will be an orphaned process that can't be stopped with rubocop-daemon stop.

To fix this, I added a file lock, so that only one process will start. If a process can't acquire the lock, then it will wait until the other process has started.

This works for both restart and start, because I've also refactored the restart command - Now it just calls stop and then start.

…status on both the client and the server, which will make sure we catch any bugs, crashes, broken pipes, etc.
Copy link
Owner

@fohte fohte left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I should give big thanks to @ndbroadbent, your bug fixes and new features are so great!
They would improve extremely the quality of rubocop-daemon.

Also, your comments are very clear, so I was easy to know the purpose of your changes, thanks!

@fohte fohte merged commit 3e41a33 into fohte:master Dec 16, 2018
@ndbroadbent ndbroadbent deleted the autostart-server branch December 18, 2018 14:29
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.

2 participants