Plural method params vs singular method names and vice versa

[Reinmar]

There is a thing which I don't like in our API and which have just reappeared in ckeditor/ckeditor5-engine#1209 – is the usage of singular method names for plural-params or vice versa:

setAttribute( { foo: 1, bar: 2 } ); // appeared in #1209
removeAttribute( [ 'a', 'b' ] ); // not proposed in #1209 but would be forced by #1209's convention
element.appendChildren( oneNode ); // ekhm...
view.registerChildren( oneView ); // ...

I'm not saying that we have all these methods now (especially after the refactoring), but some of these things appear, appeared or would need to appear in the future if we'll go this undefined way for a longer time.

The biggest problem that we have now is that ckeditor/ckeditor5-engine#1209 actually breaks the consistency that we had so far. So far we had plural method names which also accepted singular params (e.g. appendChildren() here in the engine and registerChildren() in the UI lib). This was rather ok. Now, ckeditor/ckeditor5-engine#1209 introduces setAttribute() which accepts plural params. It kinda makes sense in that case, but the overall picture gets really dirty ;/

I think that we need to clean this up. I'm not sure which direction is best so I won't propose anything for now. Thoughts?

[oleq]

e.g. appendChildren() here in the engine and registerChildren() in the UI lib

It makes sense to me. Most methods get an iterator–as–an–argument interface anyway since the project gets bigger and bigger and more use–cases arrive. That's why the general–to–specific naming convention feels like a natural choice:

registerChildren( child )

and

registerChildren( childA, childB )

a just fine to me, while

registerChild( childA, childB )

looks just weird.

Personally, I see nothing super–wrong in

element.appendChildren( oneNode ); // ekhm...
view.registerChildren( oneView ); // ...

but you know, it's just me ;-)

[scofalik]

Whenever a method can do something with multiple items it should have a plural name.

element.appendChildren( oneNode ) is perfectly fine. I think that people are used to being able to pass one item or an array of items to a method that is written in plural form. It is fine because you want to "append those things as children". One item is a special case of "any number of items bigger than 0".

I don't like setAttribute( { foo: 1, bar: 2 } ); though. This is the other way. "Multiple items" is not a special case of "one item". This is just wrong.

[pjasiun]

https://github.com/ckeditor/ckeditor5-engine/issues/1209 is using singular because we decided couples days before that https://github.com/ckeditor/ckeditor5-engine/issues/1198 will use singular. And it uses singular because view consumable use singular too.

The problem is that DOM uses singular for styles. We decided then that we will use also singular for this property in the definition because otherwise, we would have a strange naming conflict. We also decided to be consistent there and use singular class and attribute also when multiple attributes are defined or consumed. The problem is that when part of the code will use singular attribute (view definition) and part will use the plural (model selection) we will have inconsistency at some point (view definition vs. view elements, view element vs. model elements or model elements vs. model selection).

This is why I believe we should plural attributes everywhere, including view definition and consumables, and, of course, use plural everywhere, cause I agree that we need to have a convention for this.

To be consistent we should use also classes in view definition and view consumables.

The only thing I am not sure about is what to do with style. It's pretty common to use just style to talk about all style properties. Style is used to name overall style, not a specific property, so it would be fine for me do have an exception here and follow DOM conversion.

[Reinmar]

The only thing I am not sure about is what to do with style. It's pretty common to use just style to talk about all style properties. Style is used to name overall style, not a specific property, so it would be fine for me do have an exception here and follow DOM conversion.

Which piece of API do you mean here? Do we use "style" word somewhere? There's setStyle().

This is why I believe we should plural attributes everywhere, including view definition and consumables, and, of course, use plural everywhere, cause I agree that we need to have a convention for this.

To be consistent we should use also classes in view definition and view consumables.

You mean ViewElement#addClasses() too?

I started thinking about all these methods which we would need to make plural when following this convention (add/remove/set x Children/Styles/Classes/Attributes/Rules/Items/Names) and I'm getting unsure whether plural names won't make our API weird. Plus, usually, you start by implementing a singular-thing method and everytime we'd like to also allow doing multiple things at once we'd need to rename it... So I'm starting thinking that:

• we should use singular names because the basic use is singular,
• or we should have separate methods for plural and singular use.

Using plural names by default is strange because that's usually not your starting point. That's not the atomic functionality which you add in the first place. We'd already have a lot of problems with that. It's also not common in native APIs or other APIs I can think of.

OTOH, when defining schema I used properties like def.allowAttributes even though it supports singular attr too just because it was so natural to use a plural name... so perhaps a "common sense + follow your gut" rule is unavoidable here anyway.

[pjasiun]

we should use singular names because the basic use is singular,

Using plural names by default is strange because that's usually not your starting point. That's not the atomic functionality which you add in the first place. We'd already have a lot of problems with that. It's also not common in native APIs or other APIs I can think of.

I agree. In fact, it was my first thought. I find it weird that we have appendChildren method, even though usually you append one child.

Also, we talked F2F and we realized that for definitions (like schema, consumables or view element definition) plural makes more sense.

It means that we should do exactly opposite what we do now (singular definitions, plural methods).

[Reinmar]

To sum up – most of the time, the rule will be – singular method names, plural properties.

The reasoning behind singular methods: when implementing some feature, you should KISS which means that most of the time the atomic functionality which you'll add is a singular use case. Plural use usually comes second.

This doesn't apply to properties. In these cases, most often, you define some shape, some object which has 1+ things of a certain kind. And element has attributes, classes, children, etc. It's known from the very beginning. It's natural to think of these things as plural.

Take Template for instance (which got it right):

	new Template( {
		tag: 'p',
		// SINGULAR WOULD BE WEIRD HERE.
		attributes: {
			// SINGULAR BECAUSE IT'S A NAME OF THE ATTRIBUTE.
			class: [ 'foo', 'bar' ],
			style: {
				backgroundColor: 'yellow'
			}
		},
		on: {
			click: bind.to( 'clicked' )
		},
		// IT'S ALWAYS PLURAL.
		children: [
			'A paragraph.'
		]
	} ).render();

It's a tricky scenario, but it underlines that the semantic matters. An element has attributes. There's a class attribute.

Also, it shows that certain things always should be plural in such cases. Imagine what would happen if we decided to go with singular property names – there would be a completely misleading child property.

More cases where we went with the plural for property names and which worked fine – ACF in CKEditor 4, SchemaDefinition in CKEditor 5.

Some things which we'll need to change: MatcherPattern, view ElementDefinition, ViewConsumable. In all these cases we should have plural classes, styles and attributes.

Note the very delicate difference between template.attributes.class and viewElementDefinition.classes.

[Reinmar]

BTW, @pomek – please start this task from gather list of methods and properties which we should rename.

[pomek]

I assume that all (append|insert)Children are correct.

The places listed below need to be changed:

• engine/view/matcher~Matcher
  • add( ...pattern ) – attribute, class, style (code)
• engine/view/elementdefinition~ElementDefinition
  • attribute, class, style (docs)
• engine/conversion/viewconsumable~ViewConsumable (docs and code)
  • add( element, consumables ) – consumables.(attribute|class|style) (code)
  • test( element, consumables )
  • consume( element, consumables )
  • revert( element, consumables )
  • consumablesFromElement( element )
• engine/conversion/viewconsumable~ViewElementConsumables (mostly docs)
  • #_consumables
  • add( consumables )
  • test( consumables )
  • consume( consumables )
  • revert( consumables )
  • _add( type, item )
  • _test( type, item )
  • _consume( type, item )
  • _revert( type, item )

I'm not sure about the (up|down)cast-converters. There is a class attribute in options. I guess it's about the view (and DOM) layer and I'm not sure whether the changes are required too.

• engine/conversion/upcast-converters~upcastElementToAttribute
  • config.view – class, style
• engine/conversion/upcast-converters~upcastElementToMarker
  • config.view – attribute
• module:engine/view/writer~Writer
  • createAttributeElement( name, attributes, options = {} ) – docs says:

  writer.createAttributeElement( 'span', { class: 'myMarker' }, { id: 'marker:my' } );
  

  During the refactoring, class should be renamed to classes but it's about the view.

I didn't find anything in the ui package, so only the engine needs changes.

[Reinmar]

Why do we have writer.setAttributes() and writer.setAttribute()? Why not a single method like in other places? cc @pjasiun

[Reinmar]

As for the UI – View#registerChildren() -> View#registerChild(), View#deregisterChildren() -> View#deregisterChild() because these methods accept singular and plural items.

[pomek]

After closing this ticket – branch t/742 should be removed. It is used to check the CI for all repositories.

[Reinmar]

Done! Great job, @pomek.