search

ckeditor / ckeditor5-design

☠ Early discussions about CKEditor 5's architecture. Closed now. Go to https://github.com/ckeditor/ckeditor5 ☠
58 stars 12 forks source link

Use lower case for const #148

Closed pjasiun closed 8 years ago

pjasiun commented 8 years ago

Some time ago we decided to resign using const definitions:

renderer.markToSync( renderer.CHILDREN, node )

and use strings instead:

renderer.markToSync( 'CHILDREN', node )

It seems to be a good decision. We decided then to use uppers case strings, what was natural choice.

The problem is we use lower case in events names, so there is: change:children event.

This is why part of const, which is base of the event name, or the result of the event, already is lower (camel) case, see: https://github.com/ckeditor/ckeditor5-engine/blob/baa6825536a9e8647acbd8c2d0e6503dbf177d6b/src/model/document.js#L313-L322 or https://github.com/ckeditor/ckeditor5-engine/blob/baa6825536a9e8647acbd8c2d0e6503dbf177d6b/src/view/document.js#L273-L275

renderer.markToSync( 'children', node )

To avoid needless complications of our code and live we should agree to use the same conversion for event names and consts. They do not look that bad after all.

Reinmar commented 8 years ago

I'm not very happy that we'll be changing this, but I admit that lower case strings will be better. @oleq had the same case in the UI when a lowercased value could be used in the DOM directly, but it had to be transformed due to the current naming scheme.

So, +1.

scofalik commented 8 years ago

Meh, on one hand I am for the current solution. Constants should be uppercase to differentiate from normal values that you can just pass as parameter. They mean that they are only limited number of options that you can pass. That feels much more natural.

But if it is going to make some problems (and I can imagine - i.e. trying to use part of the event name as a "constant" parameter will require using toUpperCase, etc.) I am for having lowercase constants.

oleq commented 8 years ago

Just like @Reinmar mentioned, I'm for it.

pjasiun commented 8 years ago

But if it is going to make some problems (and I can imagine - i.e. trying to use part of the event name as a "constant" parameter will require using toUpperCase, etc.) I am for having lowercase constants.

It is not a matter of simply toUpperCase. You have to change MY_CONST to and back from myConst.

scofalik commented 8 years ago

Yeah, that's even worse. Let's go with lowercase constants then.

Reinmar commented 8 years ago

I updated the code style guide: https://github.com/ckeditor/ckeditor5-design/wiki/Code-Style/_compare/465585a6f20c6135f53bb2696ed7d252ff7e3c42...c5276b3bec1bcd7a8f8408c2cf0be0e3d6735417