-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Compatibility layers for v0.12 plugins #912
Conversation
9e5813d
to
6529a5b
Compare
6529a5b
to
15febd2
Compare
I've done for almost all tasks for v0.14 plugin APIs.
|
I think startup/shutdown sequence need this hack: https://gist.github.com/unak/f36def728e8c5411eff8 |
@repeatedly @sonots Anyway, could you review this change? |
15febd2
to
e3f04ff
Compare
@buffer.symlink_path = @symlink_path if @symlink_path | ||
if @symlink_path && @buffer.respond_to?(:path) | ||
(class << @buffer; self; end).module_eval do | ||
prepend SymlinkBufferMixin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prepend
doesn't work on Ruby 1.9 so this code should not be backported to v0.12, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't add label v0.12
on this pull-request... and this hack is required because Buffer plugin of v0.14 API doesn't have symbolic link feature.
There's no reason to backport this change to v0.12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is this plugin is also used in v0.12.
So if someone writes the patch to v0.12 / v0.14 out_file, v0.14 changes may conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conflicts may occur if someone want to change L90 (in previous version), but any other changes will not conflict this change.
|
||
secconf = CompatOutputUtils.secondary_section(conf) | ||
if secconf | ||
if secconf['type'] && !secconf['@type'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines could be moved to secondary_section method?
And I want to log warning like 'type' is deprecated. 'type' parameter will be ignored since v1. Use @type instead
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO changing conf
object should be done in configure
method explicitly, not in CompatOutputUtils.secondary_section
(in implicit way), for readability.
Showing type
deprecation looks good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my view, this modification is for backward compatibility, not actual plugin's behaviour.
So helper method can hide such dirty code from plugin code.
I think this is also readable for non-core developers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it should be readable only for core-developers, and I think it's easy to understand for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think easy to understand for me.
@sonots What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not so strong opinion, but if I were you, I put them into secondary_section
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an another method for obsoleted type
handling.
I will merge this change after CI green unless further comments exist. |
* change internal APIs to emit events to plugins/routers with newer one * remove all output chains from cores
dacbfff
to
a424505
Compare
This change provides compatibility layers for v0.12 style plugins of:
And this change also replaces default buffer plugins to v0.14 implementations.