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

T/85: Use a simpler plugin naming scheme #95

Merged
merged 4 commits into from
Jul 4, 2017
Merged

T/85: Use a simpler plugin naming scheme #95

merged 4 commits into from
Jul 4, 2017

Conversation

pomek
Copy link
Member

@pomek pomek commented Jul 4, 2017

Suggested merge commit message (convention)

Feature: PluginCollection will warning if a user wants to load two or more plugins with the same name. Closes ckeditor/ckeditor5#2914.


Additional information

@pomek pomek requested a review from Reinmar July 4, 2017 06:18
@Reinmar
Copy link
Member

Reinmar commented Jul 4, 2017

I think that this change is overcomplicated.

There's one place where every loaded plugin must be passed through – the _add() function. Add a warning there and it will be a 3 LOC change.

@pomek
Copy link
Member Author

pomek commented Jul 4, 2017

What about the constructor and availablePlugins? Is it also should be checked?

@Reinmar
Copy link
Member

Reinmar commented Jul 4, 2017

What about the constructor and availablePlugins? Is it also should be checked?

The question is – what we want to check. And the answer is – whether there are some duplications between loaded plugins. Why checking anything else? Always do the minimal thing but not less.

@pomek
Copy link
Member Author

pomek commented Jul 4, 2017

@Reinmar, could you look it now? I simplified the code a bit.

@Reinmar
Copy link
Member

Reinmar commented Jul 4, 2017

log.warn() should not be used with a dynamically set message because it's designed to be shortened during building (hence, it needs an id).

Also, you forgot to update the PluginInterface.pluginName documentation.

I'm on it.

@pomek
Copy link
Member Author

pomek commented Jul 4, 2017

Thx for helping.

@Reinmar Reinmar merged commit e00a282 into master Jul 4, 2017
@Reinmar Reinmar deleted the t/85 branch July 4, 2017 12:06
@Reinmar
Copy link
Member

Reinmar commented Jul 4, 2017

There were some outdated plugin names used in manual tests of the clipboard and upload packages.

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.

Use a simpler plugin naming scheme
2 participants