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

Add --stdin-file-path option #125

Merged
merged 2 commits into from May 13, 2021
Merged

Add --stdin-file-path option #125

merged 2 commits into from May 13, 2021

Conversation

greenfork
Copy link

Refs #50

Hi, decided to add this option

Copy link
Owner

@sds sds left a comment

Choose a reason for hiding this comment

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

Overall supportive, just some small questions.

end.flatten
else
collect_lints($stdin.read, options[:stdin_file_path], linter_selector, config)
end

SlimLint::Report.new(lints, files)
Copy link
Owner

Choose a reason for hiding this comment

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

It seems like we would need to ensure files is correctly updated to use the provided stdin_file_path, which is missing here. Have we tested this? What do the reports show?

Copy link
Author

Choose a reason for hiding this comment

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

Could you clarify how exactly should it be updated? Currently with stdin files = extract_applicable_files(config, options) returns an empty array.

This is how it works:

$ cat t.slim
divdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdivdiv
  = 1
$ cat  t.slim | bundle exec ./bin/slim-lint --stdin-file-path index.slim
warning: parser/current is loading parser/ruby27, which recognizes
warning: 2.7.3-compliant syntax, but you are running 2.7.2.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
index.slim:1 [W] LineLength: Line is too long. [105/80]

or in short

index.slim:1 [W] LineLength: Line is too long. [105/80]

Copy link
Owner

Choose a reason for hiding this comment

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

Right, you don't want to pass an empty files array to SlimLint::Report.new(lints, files). You would want to pass a single item array containing [options[:stdin_file_path]], i.e. files = [options[:stdin_file_path]].

See the definition for the Report class. It's expecting the list of files that were linted:

# List of files that were linted.
attr_reader :files

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I changed that files variable as you asked. For your information, it didn't change the output, everything is still same.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, that's probably because the default reporter doesn't use files. But it is used by other reporters.

@@ -19,9 +19,14 @@ def run(options = {})

Copy link
Owner

Choose a reason for hiding this comment

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

Could we skip calling extract_applicable_files since it isn't necessary if you're passing in a single file via standard input?

@greenfork greenfork requested a review from sds May 8, 2021 02:50
@greenfork
Copy link
Author

Let me know if you want any squashing of commits

@sds sds merged commit 82fe833 into sds:master May 13, 2021
@sds sds mentioned this pull request May 13, 2021
@greenfork greenfork deleted the add-stdin-file-path-option branch May 13, 2021 03:16
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

2 participants