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

Introduced Model#createOperationFromJSON #1815

Merged
merged 2 commits into from
Jan 29, 2020
Merged

Introduced Model#createOperationFromJSON #1815

merged 2 commits into from
Jan 29, 2020

Conversation

scofalik
Copy link
Contributor

Suggested merge commit message (convention)

Feature: Introduced Model#createOperationFromJSON which is an alias for OperationFactory#fromJSON. Closes ckeditor/ckeditor5#6094.


Additional information

I actually used OperationFactory inside Model#createOperationFromJSON -- less hassle and more backward compatible. If you don't like this solution, I can remove OperationFactory and fix related places (I think it was used in three places in our codebase).

@coveralls
Copy link

coveralls commented Jan 23, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 5238082 on i/6094 into b2a8189 on master.

@scofalik
Copy link
Contributor Author

scofalik commented Jan 27, 2020

This change was already tested and accepted by Cloud Services team.

src/model/model.js Outdated Show resolved Hide resolved
@Reinmar
Copy link
Member

Reinmar commented Jan 29, 2020

I actually used OperationFactory inside Model#createOperationFromJSON -- less hassle and more backward compatible. If you don't like this solution, I can remove OperationFactory and fix related places (I think it was used in three places in our codebase).

What do you think I might not like in using operation factory here? Is there some other option?

@Reinmar Reinmar merged commit ebaa2cc into master Jan 29, 2020
@Reinmar Reinmar deleted the i/6094 branch January 29, 2020 07:19
@Reinmar
Copy link
Member

Reinmar commented Jan 29, 2020

OMG, I realised that it should be fromJson everywhere... Similarly to toHtml/fromHtml.

@Reinmar
Copy link
Member

Reinmar commented Jan 29, 2020

image

🤣

@Reinmar
Copy link
Member

Reinmar commented Jan 29, 2020

Fixed in c102fc3. I fixed the same thing in the merge message before but I didn't notice the same mistake in the docs. Static methods need to be referenced ad .foo, not #foo.

@scofalik
Copy link
Contributor Author

I used createOperationFromJSON because 1) that's how it was used in OperationFactory, two, AFAIR, toJSON is the name of the method for objects serialization.

I wanted to go with Json initially but then I realized it is upper-case in OperationFactory already.

@scofalik
Copy link
Contributor Author

What do you think I might not like in using operation factory here? Is there some other option?

Another option would be to move the whole functionallity to the Model class. And not keep separate OperationFactory.

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

Successfully merging this pull request may close these issues.

Move OperationCompressor functionallity to a method on editor.model
3 participants