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

[Fix #1682] Add STDIN support #2146

Merged
merged 1 commit into from
Aug 20, 2015
Merged

[Fix #1682] Add STDIN support #2146

merged 1 commit into from
Aug 20, 2015

Conversation

caseywebdev
Copy link
Contributor

Without STDIN support, text editor plugins have to create tmp files to implement lint-as-you-type because the actual file is not saved yet. This is far from ideal, and this PR aims to fix that. Here's example usage:

$ echo '{:foo => :bar}' | bin/rubocop -s the-file-path.rb
Inspecting 1 file
C

Offenses:

the-file-path.rb:1:1: C: Missing utf-8 encoding comment.
{:foo => :bar}
^
the-file-path.rb:1:1: C: Use snake_case for source file names.
{:foo => :bar}
^
the-file-path.rb:1:1: C: Space inside { missing.
{:foo => :bar}
^
the-file-path.rb:1:2: C: Use the new Ruby 1.9 hash syntax.
{:foo => :bar}
 ^^^^^^^
the-file-path.rb:1:14: C: Space inside } missing.
{:foo => :bar}
             ^

1 file inspected, 5 offenses detected

The file path is still used for things like source file name linting, but the raw source comes from STDIN.

@steelbrain
Copy link

@caseywebdev does rubocop search for any rc files? 'cause if it does, You might have to add an extra parameter like --stdin-filepath referring to the path of the stdin input so it can resolve the imports/rc files.

@caseywebdev
Copy link
Contributor Author

@steelbrain The path is passed just the same as it always is, as the last command line argument. The -s/--stdin flag just tells the runner to ignore the contents of the file and use the content of STDIN.

See the example above.

@steelbrain
Copy link

Although this is not my repo, but it would be nice if you would add some tests to it.

@caseywebdev
Copy link
Contributor Author

I would, but the rspec DSL makes me crazy.

@tfausak
Copy link

tfausak commented Aug 15, 2015

👍

@jonas054
Copy link
Collaborator

@caseywebdev I think you should add validation for the new option, checking that exactly one file argument is given. And you need to add testing. Even if you're not used to RSpec.

@caseywebdev caseywebdev force-pushed the stdin branch 2 times, most recently from 33cb047 to db16986 Compare August 16, 2015 13:52
@caseywebdev
Copy link
Contributor Author

@jonas054 I've added tests

@jonas054
Copy link
Collaborator

The code looks good to me. You just need to add the option to the Command flag table on the README page. Mention that it's useful for editor integration.

@caseywebdev
Copy link
Contributor Author

Added -s/--stdin to the command flag table with a note about editor integration.

@jonas054
Copy link
Collaborator

👍 I'm satisfied. We'll see if @bbatsov has more comments.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 18, 2015

Apart from the failing build - the code looks good to me.

@caseywebdev
Copy link
Contributor Author

Whoops, forgot to update the help output spec. It should pass this time.

@@ -60,6 +60,40 @@ def INVALID_CODE
END
end
end

context 'if -s/--stdin is used with an offense' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where do you pass args to the runner?

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 18, 2015

I'd also like to ask you to change the commit message's title to [Fix #1682] Add STDIN support. You can mention the AtomLinter ticket and some extra explanations in the commit's body.

@caseywebdev
Copy link
Contributor Author

Done

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 18, 2015

One more thing I noticed too late - you need to add a changelog entry.

@caseywebdev
Copy link
Contributor Author

Done

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 18, 2015

And the build failed again.

@caseywebdev
Copy link
Contributor Author

Whoops, my bad. Should work now.

@caseywebdev caseywebdev changed the title Fix #1682, Re AtomLinter/linter-rubocop#54, Add STDIN support [Fix #1682] Add STDIN support Aug 18, 2015
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 20, 2015

This has to be rebased on top of the current master.

@caseywebdev
Copy link
Contributor Author

Done

bbatsov added a commit that referenced this pull request Aug 20, 2015
@bbatsov bbatsov merged commit fd56bf5 into rubocop:master Aug 20, 2015
@caseywebdev
Copy link
Contributor Author

🎆

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.

5 participants