Warnings API for deprecated features by raphamorim · Pull Request #918 · qunitjs/qunit

raphamorim · 2016-01-12T07:41:30Z

Ref: #881
Please don't merge this PR yet

I will finalize the PR if you have no problems with this implementation.

leobalter · 2016-01-12T15:56:11Z

the console.warn call needs to be abstracted. Otherwise you'll need to check for it every time you trigger a warning.

Also, the messages could be stored on a dictionary object, easier to keep.

I tried to implement this idea in aee09a2, what do you think?

leobalter · 2016-01-12T16:27:35Z

We should focus initially on the warning abstracted implementation and then on the places we should have it.

The implementation should consider:

It can be toggled off (probably trough the config object)
Avoid changing any other external part of the API, including function signatures
It might - or not - be exposed (maybe through a QUnit.warning method). It's good if it's easy to export.
The deprecation messages can stored on a object, easier to find and change them if necessary. (e.g.: QUnit.warning.deprecated)

In the mean time, I should provide a complete list containing all the deprecated parts, including:

asyncTest
Module hooks setup and teardown calls
Partial deprecation of QUnit.start
QUnit.stop
QUnit.init
QUnit.push
QUnit.reset (this might need an extra discussion before removal, but it's deprecated)
QUnit.test exposal to the global object
Qunit.module exposal to the global object
Assert methods on the QUnit and global objects
Logging callbacks being assigned with new values instead of called as functions
testDone duration property

Needs discussion or implementation:

test with the expected parameter signature
test with the async parameter signature
the callback methods after we get the EventEmitter API done

I need to check if anything is missing but this list is at least closer to reflect all the pre-2.0 deprecations.

raphamorim · 2016-01-18T09:37:32Z

Sorry about delay

leobalter · 2016-01-18T18:02:46Z

you may blend this warningMessage to the if statement above. It cost less to check for it before at first too.

Updated: a6a8c1b

raphamorim · 2016-01-20T18:58:26Z

I agree with you that's not abstract. But in the case of the test method I would like to take this message to use in new Test(), to get this warning in done()

Like:

new Test({
   warning: warningMessage,

So the best approach is:

var warning = QUnit.warning.deprecated.test;
QUnit.warning( "test" ); // or "qunit-test"

Or: In addition to print, return the message

var warning = QUnit.warning( "test" );

leobalter · 2016-01-28T17:03:48Z

We may avoid the new warning variable calling the function directly with it, less use of memory.

Also: I still believe it's better to map the messages as string keys, the point is that we may want to trigger only once for each warning. If you find another way to solve this (the single trigger) you don't need to worry on the key map.

leobalter · 2016-02-02T19:28:36Z

Ouch! some bugs found on legacy browsers. https://travis-ci.org/jquery/qunit/builds/106548000

The Warning API is getting great, we're close to move this forward.

raphamorim · 2016-02-10T02:05:00Z

@leobalter can you check out the latest changes?
I will see this bug in IE7 and start writing tests for this PR :)

leobalter · 2016-02-16T23:32:33Z

I need two udpates:

a rebase with the master branch.
please make each warning trigger only once. I can't even check the CI the way it is. :)

Extra (probably for an extra PR in the future): you may try using QUnit.stack to mention where the first of each kind of deprecation has happened.

raphamorim · 2016-02-17T03:15:27Z

Updated @leobalter

leobalter · 2016-02-18T23:38:34Z

We are close. Now we need to disable the warnings on the regular runs.

Maybe a QUnit.warning.off() being called on the main html test files (at least on test/index.html). Then, exclusive tests to check every warning (running on an isolated files, maybe a legacy-warnings.html and legacy-warnings.js).

The other improvements might come after these changes, potentially on another PR.

gibson042 · 2016-02-19T14:23:35Z

 			}
 		}

+		QUnit.warning( "testDone-duration" );


Warnings shouldn't be generated when there's nothing the user can do about it. I'd like to see this appear only in a getter for duration where Object.defineProperty exists.

raphamorim · 2016-02-23T13:00:54Z

@leobalter about the tests for QUnit.warning, do you think it's better to create a separate file (warning.html for example) to test individual cases?

leobalter · 2016-02-23T15:23:13Z

yes. I wanna make sure we trigger warnings only on isolated cases.

This will help us to update the main tests before the next major release.

leobalter · 2016-02-23T17:18:19Z

 	});

 	require( [ "qunit", "./amd" ], function( QUnit, tests ) {
+		QUnit.warning.off();


these specific tests should not worry about triggering it off. That should occur only for the main test or anything else that are still covering deprecated API.

raphamorim · 2016-02-25T02:28:40Z

@leobalter see the lastest updates. Chek if this tests make sense to you?

leobalter · 2016-04-19T17:01:34Z

+		return QUnit[ key ].apply( this, arguments );
+	};
+}
+


this should be removed

raphamorim · 2016-05-03T18:05:54Z

@leobalter check last changes :)

leobalter · 2016-05-03T18:12:38Z

there's still some pending comments above, like: #918 (comment)

trentmwillis · 2016-05-06T16:16:28Z

 		times = {};

-	( function() {
+	(function() {


Undo? Seems like an accidental change.

Updated: 21cd239

trentmwillis · 2016-05-06T16:33:29Z

Overall this seems okay to me, just a few minor comments

trentmwillis · 2016-12-04T19:30:39Z

Sorry for letting this sit for so long. @leobalter is this still desirable/needed? I wasn't super involved here, so I'm unsure what the entire goal of the API was. I'd be fine to just use a simple warning as well for things like #986.

leobalter · 2016-12-06T05:01:09Z

Makes more sense for now. Looking at this seems the warnings API is overkill at this point.

@raphamorim I'm closing this for now, thanks for the hard work and I'm sorry to not help with it so much.

raphamorim · 2016-12-06T19:20:35Z

No problemo :)

jquerybot added the CLA: Valid label Jan 12, 2016

leobalter added the in progress label Jan 12, 2016

leobalter changed the title ~~Warnings for depreacted features~~ [WIP] Warnings for depreacted features Jan 12, 2016

leobalter reviewed Jan 12, 2016
View reviewed changes

leobalter reviewed Jan 18, 2016
View reviewed changes

leobalter reviewed Jan 28, 2016
View reviewed changes

leobalter mentioned this pull request Feb 16, 2016

Test: Deprecate the 'expected' argument of QUnit.test #885

Closed

raphamorim force-pushed the feature/warnings-for-depreacted-features branch 2 times, most recently from 0de40ca to 2d6e227 Compare February 17, 2016 03:14

leobalter changed the title ~~[WIP] Warnings for depreacted features~~ Warnings for depreacted features Feb 18, 2016

gibson042 reviewed Feb 19, 2016
View reviewed changes

Krinkle changed the title ~~Warnings for depreacted features~~ Warnings for deprecated features Feb 21, 2016

leobalter reviewed Feb 23, 2016
View reviewed changes

raphamorim force-pushed the feature/warnings-for-depreacted-features branch 2 times, most recently from ea7a6a0 to 2191cfc Compare February 25, 2016 02:37

leobalter reviewed Apr 19, 2016
View reviewed changes

Comment thread src/export.js Outdated

return QUnit[ key ].apply( this, arguments );

};

}

leobalter Apr 19, 2016

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

this should be removed

leobalter changed the title ~~Warnings for deprecated features~~ Warnings API for deprecated features Apr 20, 2016

trentmwillis mentioned this pull request Apr 21, 2016

Assert: Deprecate assert.push #992

Merged

Test: Removing warning.off

f7bd904

raphamorim added 2 commits May 3, 2016 16:22

Test: Making some adjustments

82ddcb3

Core: Adjustments on code style

f1aabfc

trentmwillis mentioned this pull request May 6, 2016

Component: Assert.push #994

Closed

trentmwillis reviewed May 6, 2016
View reviewed changes

Test: Adjust code style

21cd239

leobalter closed this Dec 6, 2016

raphamorim deleted the feature/warnings-for-depreacted-features branch December 6, 2016 19:20

leobalter mentioned this pull request Dec 29, 2018

Document deprecated API #1352

Closed

Conversation

raphamorim commented Jan 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leobalter commented Jan 12, 2016

Uh oh!

raphamorim commented Jan 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raphamorim commented Jan 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leobalter commented Feb 2, 2016

Uh oh!

raphamorim commented Feb 10, 2016

Uh oh!

leobalter commented Feb 16, 2016

Uh oh!

raphamorim commented Feb 17, 2016

Uh oh!

leobalter commented Feb 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raphamorim commented Feb 23, 2016

Uh oh!

leobalter commented Feb 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raphamorim commented Feb 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raphamorim commented May 3, 2016

Uh oh!

leobalter commented May 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trentmwillis commented May 6, 2016

Uh oh!

trentmwillis commented Dec 4, 2016

Uh oh!

leobalter commented Dec 6, 2016

Uh oh!

raphamorim commented Dec 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants