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

Breaking: Change .not.property(name, val) behavior #744

Merged
merged 1 commit into from
Aug 14, 2016

Conversation

meeber
Copy link
Contributor

@meeber meeber commented Jul 2, 2016

  • Previously, expect(obj).not.property(name, val) would throw an Error
    if obj didn't have a property named name. This change causes the
    assertion to pass instead.
  • assert.propertyNotVal renamed to assert.notPropertyVal
  • assert.deepPropertyNotVal renamed to assert.notDeepPropertyVal

Notes:

- Previously, `expect(obj).not.property(name, val)` would throw an Error
if `obj` didn't have a property named `name`. This change causes the
assertion to pass instead.
- assert.propertyNotVal renamed to assert.notPropertyVal
- assert.deepPropertyNotVal renamed to assert.notDeepPropertyVal
@keithamus
Copy link
Member

This LGTM 👍

@keithamus
Copy link
Member

@lucasfcosta thoughts?

@lucasfcosta
Copy link
Member

lucasfcosta commented Aug 12, 2016

LGTM too, but read this first.
Since there's no other PR which this one depends upon, I will get this one merged first, but before that, I have a doubt:
Since it is a breaking change, shouldn't this PR be made against 4.x.x? If I'm wrong, feel free to click the merge button 😄

@meeber
Copy link
Contributor Author

meeber commented Aug 12, 2016

@lucasfcosta My thought is that if everything looks good we can merge these in right before starting the 4.x.x-to-master merge.

@lucasfcosta
Copy link
Member

lucasfcosta commented Aug 12, 2016

@meeber Hmmm, I've had that doubt especially because of #751.
I was concerned that if we had that change merged before the 4.0 release we would release a breaking canary version, which would be a really undesirable thing.

@keithamus
Copy link
Member

@lucasfcosta why would releasing a breaking canary version be a problem?

@lucasfcosta
Copy link
Member

@keithamus I thought our goal with that would be just to release non-breaking versions, but including latest features and fixes, so anyone could using the canary tag in their package.json wouldn't get a build broken due to that.

However, I'm not too strict about it, if guys prefer this way I don't see any problem due to the fact that anyone using the canary tag should be aware they're using experimental and new code.

@keithamus
Copy link
Member

@lucasfcosta My assumption is that canary would include any features in the next upcoming version of chai. The next upcoming version of chai is 4.0 and as such has breaking changes. It gives folk a chance to fix their tests or brace themselves before 4.0 lands.

@meeber
Copy link
Contributor Author

meeber commented Aug 12, 2016

I'm in favor of doing things in this order:

  1. Merge these breaking changes into master
  2. Merge 4.x.x into master
  3. Release a canary version for testing by our user base
  4. Start writing migration guide and/or scripts
  5. Merge any bug fixes as needed
  6. Merge deep-eql refactor into master
  7. Release updated canary version for testing
  8. Merge any bug fixes as needed
  9. Finish writing migration guide and/or scripts
  10. Depending on number of changes, release a final updated canary version for testing
  11. Release official v4.0.0

@lucasfcosta
Copy link
Member

lucasfcosta commented Aug 14, 2016

Thank you for sharing you ideas guys, it's great to have such a great team to discuss these kinds of things 😄
I totally agree with @meeber's post.

Let's get this merged! 😄 I'll get back to reviewing the other PRs then.

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