Skip to content

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.

@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!

@kabirbaidhya kabirbaidhya requested a review from nknapp October 28, 2019 07:25
@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.

2 participants