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 missing types for the Exception class properties #1583

Merged
merged 11 commits into from Oct 28, 2019

Conversation

kabirbaidhya
Copy link
Contributor

@kabirbaidhya kabirbaidhya commented Oct 24, 2019

Resolves #1576

Add missing type declarations for the properties of Exception class.

What has changed?

  • Converted function to a class so as to declare fields.
  • Declare all the properties (with the types inferred) that have been attached to the this object in the source file.
  • Declare a missing second argument in the typing - node of type hbs.AST.Node.
  • Add tests for the exception typing.

  • 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.

@nknapp Not sure if this change requires a test too. If yes, please suggest and show me some examples and I can do it. Thanks!

PS: I tried npm checkTypes and npm test which all passed for me - not sure what else I need to do.

@nknapp
Copy link
Collaborator

nknapp commented Oct 25, 2019

Thanks for the PR. Test cases for the typings go into the types/test.ts file.

@kabirbaidhya
Copy link
Contributor Author

OK. I'll add some.

@kabirbaidhya
Copy link
Contributor Author

@nknapp I've added some code using the defined types in the types/test.ts file. Please review and let me know if it makes sense. Thanks!

Copy link
Collaborator

@nknapp nknapp left a comment

Choose a reason for hiding this comment

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

Thanks for adapting my change requests. I have now had a chance to have a closer look and I would you to make further changes. Please have a look at my review comments.

types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/test.ts Outdated Show resolved Hide resolved
types/test.ts Outdated Show resolved Hide resolved
@kabirbaidhya
Copy link
Contributor Author

@nknapp Thanks for the detail review. I'll make the changes push again.

@kabirbaidhya
Copy link
Contributor Author

Hi @nknapp I've pushed new changes as per your suggestions. Could you please review again and let me know if this looks good? Thanks!

@nknapp
Copy link
Collaborator

nknapp commented Oct 28, 2019

Superb. Merging now.

@nknapp nknapp merged commit b8913fc into handlebars-lang:4.x Oct 28, 2019
@kabirbaidhya kabirbaidhya deleted the missing-types branch October 28, 2019 16:52
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