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

feat(template-lint): Read from stdin #2622

Merged
merged 9 commits into from
Aug 31, 2020
Merged

feat(template-lint): Read from stdin #2622

merged 9 commits into from
Aug 31, 2020

Conversation

dcyriller
Copy link
Contributor

@dcyriller dcyriller commented Jun 26, 2019

Description

This PR proposes to leverage a new feature introduced in ember-template-lint v1.6: reading from stdin.

Currently, ale copies the file to a temporary file and lints it from there. But this approach has a few issues regarding the configuration files (especially .prettierrc.js).

@dcyriller dcyriller changed the title template-lint: Lint against source file not a temp file fix(template-lint): Lint against source file not a temp file Jun 26, 2019
Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

The reason the author is checking a temporary file is so they can check for problems with the buffer as you type. Does this linter support some sort of option to make this work properly? If it does, find out that that is and use it. If it doesn't, set 'lint_file': 1 so the linter only runs when the buffer is opened/saved.

@w0rp w0rp added this to In Review in Old Working List via automation Jul 6, 2019
@dcyriller
Copy link
Contributor Author

Interesting! Thank you for your guidance.

Unfortunately, there is no such option with this linter. I've added the 'lint_file': 1 option.

@dcyriller
Copy link
Contributor Author

It should be good now :)

@w0rp w0rp added this to In Review in On the Radar Aug 17, 2019
@dcyriller
Copy link
Contributor Author

Reading from stdin is currently being added in template-lint. Maybe we should wait for that feature and then update this PR.

@w0rp
Copy link
Member

w0rp commented Sep 10, 2019

Okay, let's wait for that then. Sorry for the delay.

@dcyriller
Copy link
Contributor Author

Cross-linking the wip for future reference: ember-template-lint/ember-template-lint#783

@dcyriller
Copy link
Contributor Author

The above-mentioned PR has been merged in ember-template-lint.

I've updated this PR to lint from stdin. It should be good now @w0rp!

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Update the tests to handle this. I think we should add a --version check for this linter too, so it will work with older and newer versions. ale_linters/php/phpstan.vim contains some pretty good example code for running different commands for different versions.

@@ -34,6 +34,6 @@ call ale#linter#Define('handlebars', {
\ 'executable': {b -> ale#node#FindExecutable(b, 'handlebars_embertemplatelint', [
\ 'node_modules/.bin/ember-template-lint',
\ ])},
\ 'command': '%e --json %t',
\ 'command': '%e --json < %t',
Copy link
Member

Choose a reason for hiding this comment

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

If you take < %t away from the command, ALE should pipe in stdin to the command automatically. ALE does that whenever %t isn't present in the command, and the read_buffer option isn't set to 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.

Ok, I didn’t know that. I will update accordingly.

Copy link

Choose a reason for hiding this comment

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

@dcyriller - We should also pass the current file name in via --filename (so that the linting rules that make changes based on filename have the correct value).

@dcyriller
Copy link
Contributor Author

@w0rp it should be ready for review

@dcyriller dcyriller requested a review from w0rp March 13, 2020 13:13
@dcyriller dcyriller changed the title fix(template-lint): Lint against source file not a temp file feat(template-lint): Read from stdin Apr 12, 2020
@dcyriller
Copy link
Contributor Author

dcyriller commented Apr 13, 2020

I wound need some help to understand why the CI is failing. @w0rp is there any chance you have some time to help me on this?


AssertEqual
\ ale_linters#handlebars#embertemplatelint#GetCommand(bufnr(''), [2, 0, 0]),
\ '%e --json --filename /testplugin/test/ember-template-lint-test-files/app/template.hbs'
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like:

  if has('win32')
    let g:ember_template_lint_command =
    \ '%e --json --filename C:\testplugin\test\ember-template-lint-test-files\app\template.hbs'
  else
    let g:ember_template_lint_command =
    \ '%e --json --filename /testplugin/test/ember-template-lint-test-files/app/template.hbs'
  endif

  AssertEqual
  \ ale_linters#handlebars#embertemplatelint#GetCommand(bufnr(''), [2, 0, 0]),
  \ g:ember_template_lint_command

should work.

@dcyriller
Copy link
Contributor Author

Okay CI is green now, thanks a lot for your help @jamescdavis!

This PR has been tested locally by a few people I believe @w0rp. So the changes should be safe 😄. Do you mind giving it a review?

@jamescdavis
Copy link
Contributor

Oh, that's much better than the conditional!

@jamescdavis
Copy link
Contributor

I tested locally again with this latest change and can confirm it works.

@dcyriller
Copy link
Contributor Author

friendly ping @w0rp

@jamescdavis
Copy link
Contributor

Also ping @hsanson 😉

@w0rp
Copy link
Member

w0rp commented Aug 31, 2020

This doesn't work because @% is the current filename, but the current filename might not be the same file as the one that ALE started checking earlier. I'll fix this by using the %s formatting ALE has.

ale_linters/handlebars/embertemplatelint.vim Outdated Show resolved Hide resolved
test/test_embertemplatelint_executable_detection.vader Outdated Show resolved Hide resolved
@w0rp w0rp merged commit d4a1474 into dense-analysis:master Aug 31, 2020
On the Radar automation moved this from In Review to Done Aug 31, 2020
@w0rp
Copy link
Member

w0rp commented Aug 31, 2020

Cheers! 🍻

@dcyriller dcyriller deleted the template-lint-remove-temp-file branch August 31, 2020 10:58
@dcyriller
Copy link
Contributor Author

Thanks a lot @w0rp !

jsit added a commit to jsit/ale that referenced this pull request Apr 19, 2021
* master: (123 commits)
  Add tests for maven.vim file
  Fix grammatical error in doc
  Add maven helper file; use maven wrapper if available instead of global 'mvn' executable
  fix lint, fix variable semantics and update tests
  bibclean: update matchlist reges for bibclean > v2.11.4
  Bump the ALE version to 3.0.0
  Close dense-analysis#2522 - Check pylint on the fly
  Add tests for covering the coming Vint version
  Close dense-analysis#3003 - Show ignored linters in :ALEInfo
  Close dense-analysis#3333 - Add an ALECompletePost event
  Fix a completion error
  Fix typo
  Close dense-analysis#3268 - Implement :ALEImport
  Fix dense-analysis#3183 - Escape filename characters from LSP/tsserver
  Clean up embertemplatelint code
  Fix asciidoc languagetool integration
  Fix dense-analysis#3322 - Apply rename changes correctly
  feat(template-lint): Read from stdin (dense-analysis#2622)
  Fix flake8 cd logic for invalid options
  Add tests for \r removal
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants