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

@zimmi88
Copy link
Contributor

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator

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 wycats: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

This comment has been minimized.

Copy link
Collaborator

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
Projects
None yet
2 participants
You can’t perform that action at this time.