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

Port over linting and test for typings #1515

Merged
merged 1 commit into from Mar 27, 2019

Conversation

@timlindvall
Copy link
Contributor

@timlindvall timlindvall commented Mar 19, 2019

  • Move typings from lib/ to types/.
  • Add dtslint for validating types.
  • Use grunt-bg-shell to call out to dtslint during build step.

Notes:

  • dtslint is the tooling that was used previously to check types, and it's designed to validate typings, so it seemed like a good fit for this use case. Much of the directory structure in this PR mimics the instructions from https://github.com/Microsoft/dtslint.
  • Couldn't find a grunt library to run dtslint, so I'm using grunt-bg-shell to proxy to a script in package.json that runs dtslint.
  • Originally, I ran into an issue where dtslint was complaining about types in node_modules which seemed odd. But, after making a couple of tweaks to the typings test file, I'm not able to reproduce this anymore. I'm relying on the test run against this PR to tell me if this was a fluke or if the issue persists.

=========
Before creating a pull-request, please check https://github.com/wycats/handlebars.js/blob/master/CONTRIBUTING.md first.

Generally we like to see pull requests that

  • Please don't start pull requests for security issues. Instead, file a report at https://www.npmjs.com/advisories/report?package=handlebars
  • Maintain the existing code style
  • Are focused on a single change (i.e. avoid large refactoring or style adjustments in untouched code if not the primary goal of the pull request)
  • Have good commit messages
  • Have tests
  • Have the typings (lib/handlebars.d.ts) updated on every API change. If you need help, updating those, please mention that in the PR description.
  • Don't significantly decrease the current code coverage (see coverage/lcov-report/index.html)
  • Currently, the 4.x-branch contains the latest version. Please target that branch in the PR.
- Move typings from lib/ to types/.
- Add dtslint for validating types.
- Use grunt-bg-shell to call out to dtslint during build step.
@nknapp
Copy link
Collaborator

@nknapp nknapp commented Mar 21, 2019

I think I would rather create a custom grunt task in the tasks-directory, than including another pacjage. Just use child_process.execFileSync to run dtslint.

But we can also merge your commit first, and then change the task. I'll look into it.

Thanks for supplying this.

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Mar 21, 2019

OK, never mind. I feared that grunt-bg-shell would be pulling tons of dependencies into tree. But it doesn't have any, so we might as well keep it.

@nknapp nknapp merged commit c454d94 into handlebars-lang:4.x Mar 27, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nknapp
Copy link
Collaborator

@nknapp nknapp commented Apr 13, 2019

Released in 4.1.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants