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

Fix meta property declaration to be optional #58

Closed
wants to merge 1 commit into from

Conversation

smithad15
Copy link

According to the comment immediately above the property, meta is supposed to be optional. This PR brings the type description in line with that sentiment.

According to the comment immediately above the property, `meta` is supposed to be optional. This PR brings the type description in line with that sentiment.
@coveralls
Copy link

coveralls commented Apr 17, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 6ea25b7 on smithad15:patch-1 into e55a736 on acdlite:master.

@JaKXz
Copy link
Contributor

JaKXz commented Apr 21, 2017

Thank you for the PR, but please see the discussion in #53.

@smithad15
Copy link
Author

Understood, however, this does cause some confusion between the documentation and the type enforcement. If you are hesitant to change the type declarations while waiting for TS2.3, might I suggest changing the documentation to reflect the current limitations instead? Having a property described as optional, and then enforcing its inclusion only for TypeScript users is a little counterintuitive. Personally, I've had to implement my own type description in my projects in order to work around this. In the end, it's up to you. Thanks for the library!

@JaKXz
Copy link
Contributor

JaKXz commented May 16, 2017

@smithad15 thanks for the response, and sorry for my slow response. Please feel free to submit that documentation change that is relevant until TS 2.3, but for now I'm going to close this PR in favour of #53.

cc @wub, @unional

@JaKXz JaKXz closed this May 16, 2017
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.

3 participants