Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Changed public API to protected #1328

Merged
merged 18 commits into from
Mar 9, 2018
Merged

Changed public API to protected #1328

merged 18 commits into from
Mar 9, 2018

Conversation

pomek
Copy link
Member

@pomek pomek commented Feb 27, 2018

Suggested merge commit message (convention)

Other: Methods which modify the model's and view's tree are now protected and shouldn't be used directly in the code. Instead of an instance of Writer should be used. Closes ckeditor/ckeditor5#3923.


Other repositories which should be merged along with this PR:

@pomek pomek requested a review from pjasiun February 27, 2018 15:02
@pomek
Copy link
Member Author

pomek commented Feb 27, 2018

9 tests fail because of the issue with Differ – https://github.com/ckeditor/ckeditor5-engine/issues/1326.

☝️ - fixed.

@coveralls
Copy link

coveralls commented Feb 27, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 1b83114 on t/738 into b91d967 on master.

Copy link
Contributor

@szymonkups szymonkups left a comment

Choose a reason for hiding this comment

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

Looks good to me, good job. Just minor things to fix.

@@ -25,6 +25,10 @@ export default class DocumentFragment {
/**
* Creates an empty `DocumentFragment`.
*
* **Note:** Constructor of this class shouldn't be used directly in the code. Instead of use the
* {@link module:engine/model/writer~Writer#createDocumentFragment} method.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the second sentence could be: "Use the {@link module:engine/model/writer~Writer#createDocumentFragment} method instead."

* @param {Number} index Index of the first node to remove.
* @param {Number} [howMany=1] Number of nodes to remove.
* @returns {Array.<module:engine/model/node~Node>} Array containing removed nodes.
*/
removeChildren( index, howMany = 1 ) {
const nodes = this._children.removeNodes( index, howMany );
_removeChildren( index, howMany = 1 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move toJSON and fromJSON methods above protected ones. Please check other files you modified for correct order as described here. We sometimes forget about that but let's fix that starting with files modified in this PR.

@pjasiun
Copy link

pjasiun commented Feb 28, 2018

It would be great to have @see and a link to the proper writer method next to each protected method.

@pjasiun
Copy link

pjasiun commented Feb 28, 2018

text.data should be readonly.

@pjasiun
Copy link

pjasiun commented Feb 28, 2018

All element.clone, node.clone, text.clone should be protected now too.

@pjasiun
Copy link

pjasiun commented Mar 2, 2018

Nodelist constructor should be protected.

@pjasiun
Copy link

pjasiun commented Mar 2, 2018

Please check all properties and add @readonly wherever it is needed.

I found these properties with missing@readonly flag:

  • model.document
  • model.schema
  • element.name

But there might be more.

@pomek
Copy link
Member Author

pomek commented Mar 6, 2018

@pjasiun and @szymonkups, fixes that you proposed and missing @readonly tag has been introduced. Could you guys review it once again?

I will fix the rest packages.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants