Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#16848 closed enhancement (fixed)

[patch][cla] make createSubclass() mixins parameter optional

Reported by: ben hockey Owned by: cjolif
Priority: high Milestone: 1.10
Component: Core Version: 1.8.3
Keywords: Cc: Bryan Forbes
Blocked By: Blocking:

Description (last modified by ben hockey)

since the base class is implied in createSubclass it should work without providing mixins.

SomeClass.createSubclass({ ... }); // should work

SomeClass.createSubclass([], { ... }); // the current workaround

Attachments (1)

0001-Made-declare.createSubclass-mixins-parameters-option.patch (941 bytes) - added by DevShaft 5 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by ben hockey

Owner: set to cjolif
Status: newassigned

comment:2 Changed 6 years ago by ben hockey

Description: modified (diff)

comment:3 Changed 6 years ago by Bryan Forbes

Cc: Bryan Forbes added

comment:4 Changed 6 years ago by bill

Type: defectenhancement

Would be nice, although you could also argue that declare({ ... }) should work, rather than declare([], { ... }).

comment:5 Changed 6 years ago by Bryan Forbes

That's true, however createSubclass(null, {}) doesn't even work, so it's not matching declare anyway.

comment:6 Changed 5 years ago by DevShaft

Here by I want to contribute a patch for this issue. Basically the mixins parameter is made optional same way as in the declare function. I think an extra test case is not needed for this issue. Let me know if you need anything.

EDIT: Ignore the attachment, was the wrong file. See here for patch: https://github.com/DevShaft/dojo/commit/9aab83215fb7190ff7f98ba6c2c38275c24c80f9.patch

Last edited 5 years ago by DevShaft (previous) (diff)

comment:7 Changed 5 years ago by cjolif

Hi DevShaft?,

Thanks for your contribution. A few things however:

1/ you need to sign a Dojo CLA to contribute see http://dojofoundation.org/about/cla. Have you signed one? 2/ having a test would be good. Especially that this should be easy by just extending the current createSubclass tests with this? 3/ once 1/ and 2/ are done I suggest submitting your patch as a github pull request as this is the new way to proceed (see https://github.com/dojo/dojo/blob/master/CONTRIBUTING.md)

comment:8 Changed 5 years ago by DevShaft

Hi cjolif, thanks for your reply. Yes, I have 1/ and will do 2/ and 3/.

Last edited 5 years ago by DevShaft (previous) (diff)

comment:9 Changed 5 years ago by dylan

Milestone: tbd1.10
Priority: undecidedhigh
Summary: createSubclass assumes mixins are always provided[patch][cla] createSubclass assumes mixins are always provided

comment:10 Changed 5 years ago by bill

Looks like https://github.com/dojo/dojo/pull/46 has a test case (and the fix).

comment:11 Changed 5 years ago by cjolif

thanks bill, so if nobody has objections I'm going to merge this as I own the ticket.

comment:12 Changed 5 years ago by bill

sounds good to me

comment:13 Changed 5 years ago by dylan

ok with me.

comment:14 Changed 5 years ago by cjolif

Resolution: fixed
Status: assignedclosed

comment:15 Changed 5 years ago by bill

Was pushed in d4d8cc718873da37d8026ab8b24065074ed34b2f.

comment:16 Changed 5 years ago by bill

Summary: [patch][cla] createSubclass assumes mixins are always provided[patch][cla] make createSubclass() mixins parameter optional
Note: See TracTickets for help on using tickets.