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: check target's type in .property assertion #992

Merged
merged 1 commit into from
Jun 19, 2017

Conversation

meeber
Copy link
Contributor

@meeber meeber commented Jun 10, 2017

Previously, the .property assertion failed ungracefully if the target was null or undefined. This commit causes an AssertionError to be thrown instead so that the ssfi flag and custom error messages are respected.

Previously, the `.property` assertion failed ungracefully if the target was `null` or `undefined`. This commit causes an `AssertionError` to be thrown instead so that the ssfi flag and custom error messages are respected.
@keithamus
Copy link
Member

Do we want to consider this a bugfix or a breaking change?

@meeber
Copy link
Contributor Author

meeber commented Jun 10, 2017

@keithamus I vote bugfix. The .property assertion was already incompatible with null and undefined because it was throwing a TypeError on this line. This PR just replaces the TypeError with an AssertionError.

@vieiralucas
Copy link
Member

I vote bugfix too, nobody should be relying on this throwing a TypeError anyways.

Copy link
Member

@vieiralucas vieiralucas left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@lucasfcosta lucasfcosta left a comment

Choose a reason for hiding this comment

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

LGTM too.

I think this should be a fix too, because it already failed before this change, it just did it in a bad way instead of throwing an informative error.

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.

4 participants