Skip to content
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

Do not expose both converters and helpers #4281

Closed
pjasiun opened this issue Feb 16, 2018 · 11 comments
Closed

Do not expose both converters and helpers #4281

pjasiun opened this issue Feb 16, 2018 · 11 comments
Assignees
Labels
package:engine resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior).

Comments

@pjasiun
Copy link

pjasiun commented Feb 16, 2018

Right not both "helpers" (like downcastAttributeToElement) and "converters" (like wrap) are exposed. As long as we do not need both, we should limit exports only to "helpers" which can be call "converters". Current "converters" should be just a part or it, which is not exposed as separate function. It can even be merged into the single function.

@ma2ciek
Copy link
Contributor

ma2ciek commented Mar 9, 2018

I remember, that on the latest TM some of us weren't sure about this change.

cc @scofalik

@Reinmar
Copy link
Member

Reinmar commented Mar 9, 2018

We were sure that we want to make it now, because we can always re-expose some things if needed. But we can't hide things which are published that easily.

@Reinmar
Copy link
Member

Reinmar commented Mar 9, 2018

PS. Of course, unless, when working on this task, we'll realise that we need some of them even now :D

@pjasiun
Copy link
Author

pjasiun commented Apr 5, 2018

So, in downcast-converters.jsonly these functions should be exported:

downcastElementToElement
downcastAttributeToElement
downcastAttributeToAttribute
downcastMarkerToElement
downcastMarkerToHighlight
insertText (renamed to downcastText)
remove
createViewElementFromHighlightDescriptor

While these should not be exported:

insertElement
insertUIElement
removeUIElement
changeAttribute
wrap
highlightText
highlightElement
removeHighlight

The tricky part here might be, how to modify tests to avoid using private functions.

@Reinmar Reinmar assigned pomek and unassigned ma2ciek Apr 5, 2018
@pomek
Copy link
Member

pomek commented Apr 24, 2018

how to modify tests to avoid using private functions

Not only the tests but also dev-utils/model.

@pjasiun
Copy link
Author

pjasiun commented Apr 30, 2018

Not only the tests but also dev-utils/model.

For dev-utils we may have protected API.

@Reinmar
Copy link
Member

Reinmar commented Sep 20, 2018

I'll quote myself from ckeditor/ckeditor5-engine#1556:

I'm actually unsure now about hiding them :D With time, I got the feeling that most questions about conversion are questions about some odd cases. Like, extending existing converters, overriding some things, etc. I can't tell from my experience which of the functions we need, but if these helpers are used in our converters, there's a high chance that they can help in people's scenarios.

@pjasiun
Copy link
Author

pjasiun commented Sep 21, 2018

@Reinmar
Copy link
Member

Reinmar commented Nov 1, 2018

As I thought – I actually had to use insertElement() and not downcastElementToElement(). Using insertElement() allowed me to perform some custom actions (mapping) while still reusing 90% of the logic.

@Reinmar
Copy link
Member

Reinmar commented Jan 30, 2019

I'm closing this :D We're not going to do it anyway.

@Reinmar Reinmar closed this as completed Jan 30, 2019
@pjasiun
Copy link
Author

pjasiun commented Jan 30, 2019

I believe it was fixed by ckeditor/ckeditor5-engine#1613.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added module:conversion resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior).
Projects
None yet
Development

No branches or pull requests

5 participants