-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Plural method params vs singular method names and vice versa #742
Comments
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 ;-) |
Whenever a method can do something with multiple items it should have a plural name.
I don't like |
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 This is why I believe we should plural To be consistent we should use also The only thing I am not sure about is what to do with |
Which piece of API do you mean here? Do we use "style" word somewhere? There's
You mean 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:
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 |
I agree. In fact, it was my first thought. I find it weird that we have 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). |
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 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 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 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: Note the very delicate difference between |
BTW, @pomek – please start this task from gather list of methods and properties which we should rename. |
I assume that all The places listed below need to be changed:
I'm not sure about the (up|down)cast-converters. There is a
|
Why do we have |
As for the UI – |
After closing this ticket – branch |
Other: Renamed plural method names to singular. See ckeditor/ckeditor5#742. BREAKING CHANGES: `View#registerChildren()` and `View#deregisterChildren()` have been renamed to `View#registerChild()` and `View#deregisterChild()`.
Internal: Aligned to changes in UI. See ckeditor/ckeditor5#742.
Internal: Aligned to changes in UI. See ckeditor/ckeditor5#742.
Internal: Aligned to changes in UI. See ckeditor/ckeditor5#742.
Other: Renamed plural method names to singular and singular attribute names to plural. See ckeditor/ckeditor5#742. BREAKING CHANGES: Properties in `MatcherPattern`, view `ElementDefinition` and options for conversion utils have been renamed: `class` to `classes`, `style` to `styles`, `attribute` to `attributes`.
Internal: Aligned to the changes in the engine. See ckeditor/ckeditor5#742.
Internal: Aligned to the changes in the engine. See ckeditor/ckeditor5#742.
Internal: Aligned to the changes in the engine. See ckeditor/ckeditor5#742.
Other: Aligned `ElementDefinition` usage to the changes in the engine. See ckeditor/ckeditor5#742. BREAKING CHANGES: In the custom format of the font size configuration the `view.style`, `view.class` and `view.attribute` properties are now called `view.styles`, `view.classes` and `view.attributes`.
Internal: Aligned to the changes in the engine. See ckeditor/ckeditor5#742.
Other: Aligned `ElementDefinition` usage to the changes in the engine. See ckeditor/ckeditor5#742. BREAKING CHANGES: In the custom format of the heading feature configuration the `view.style`, `view.class` and `view.attribute` properties are now called `view.styles`, `view.classes` and `view.attributes`.
Done! Great job, @pomek. |
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:
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 andregisterChildren()
in the UI lib). This was rather ok. Now, ckeditor/ckeditor5-engine#1209 introducessetAttribute()
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?
The text was updated successfully, but these errors were encountered: