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

[BUGFIX canary] Deprecate passing function as test argument to deprecate/warn/assert #12370

Merged
merged 1 commit into from
Oct 1, 2015

Conversation

e00dan
Copy link
Contributor

@e00dan e00dan commented Sep 18, 2015

This PR attempts to deprecate using function as test argument in: Ember.deprecate, Ember.warn and Ember.assert. It's first part of closing issue #11898. Tests for ember-debug pass, but I still have to fix other tests and code, which trigger deprecation with function as test argument. Seems like this behavior is used in many places in application lifecycle. It's my first attempt to commit actual code to Ember.js so I appreciate any help.

@mmun, @rwjblue

@e00dan
Copy link
Contributor Author

e00dan commented Sep 19, 2015

All tests pass. (sauce test suite pass sometimes, sometimes it needs to re-run... it is really misleading)

Also, it seems to me that packages/ember-htmlbars/lib/node-managers/view-node-manager.js isn't tested properly, so I've added basic test coverage to at least check if test condition in assert is correct.

@mmun, @rwjblue

@e00dan e00dan changed the title [WIP] Deprecate passing function as test argument to deprecate/warn/assert [BUGFIX release] Deprecate passing function as test argument to deprecate/warn/assert Sep 22, 2015
@e00dan
Copy link
Contributor Author

e00dan commented Sep 26, 2015

@rwjblue should I rebase or you aren't interested in this pr?

@stefanpenner
Copy link
Member

i would be in favor, but will defer to @rwjblue

@e00dan
Copy link
Contributor Author

e00dan commented Sep 29, 2015

@rwjblue any feedback on this?

@mmun
Copy link
Member

mmun commented Sep 29, 2015

@kuzirashi Sorry for no feedback. I'm definitely 👍 on this.

@e00dan
Copy link
Contributor Author

e00dan commented Sep 29, 2015

Thanks @mmun! I'm waiting for signal from @rwjblue and I'll eventually rebase and squash.

@@ -87,7 +87,7 @@ export let missingOptionsUntilDeprecation = 'When calling `Ember.deprecate` you
@param {String} message A description of the deprecation.
@param {Boolean|Function} test A boolean. If falsy, the deprecation
will be displayed. If this is a function, it will be executed and its return
Copy link
Member

Choose a reason for hiding this comment

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

Lets just remove the reference to using a function all together.

@rwjblue
Copy link
Member

rwjblue commented Sep 29, 2015

@kuzirashi - This looks very good. I have left a few pretty minor comments. When you have time can you fixup the comments, squash, and rebase?

@e00dan
Copy link
Contributor Author

e00dan commented Sep 29, 2015

I'll find the time. I'll mention you when it's all ready. Thanks for review. ;)

@rwjblue
Copy link
Member

rwjblue commented Sep 29, 2015

I agree with @mmun though, this deprecation should probably not be [BUGFIX release]. In order to allow addons to deal with the new deprecation in an orderly way, I'd like to land this on master before branching into 2.2 beta (which should be in the next few days). That gives folks the full beta cycle (~6weeks) to get things updated in addons so this doesn't impact end user apps much.

tldr; can you update to use [BUGFIX canary] instead?

@e00dan
Copy link
Contributor Author

e00dan commented Sep 29, 2015

Sure it'll be changed to [BUGFIX canary].

@e00dan e00dan changed the title [BUGFIX release] Deprecate passing function as test argument to deprecate/warn/assert [WIP] [BUGFIX canary] Deprecate passing function as test argument to deprecate/warn/assert Sep 29, 2015
@e00dan
Copy link
Contributor Author

e00dan commented Oct 1, 2015

Rebased, squashed, tests pass. Except tests on sauce which randomly pass or fail from my experience. @rwjblue

@rwjblue rwjblue changed the title [WIP] [BUGFIX canary] Deprecate passing function as test argument to deprecate/warn/assert [BUGFIX canary] Deprecate passing function as test argument to deprecate/warn/assert Oct 1, 2015
@rwjblue
Copy link
Member

rwjblue commented Oct 1, 2015

Awesome, thanks for your hard work on this @kuzirashi!

rwjblue added a commit that referenced this pull request Oct 1, 2015
[BUGFIX canary] Deprecate passing function as test argument to deprecate/warn/assert
@rwjblue rwjblue merged commit e64c676 into emberjs:master Oct 1, 2015
@rwjblue
Copy link
Member

rwjblue commented Oct 5, 2015

@kuzirashi - Would you mind submitting a PR to https://github.com/emberjs/website/blob/master/source/deprecations/v2.x.html.md adding a section on this for 2.2?

@e00dan
Copy link
Contributor Author

e00dan commented Oct 5, 2015

Sure :)

@e00dan
Copy link
Contributor Author

e00dan commented Oct 5, 2015

PR in website repo created. @rwjblue

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