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

Change modifier from private to protected #126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

legiangthanh
Copy link
Member

In this PR, I propose we change private to protected, There is some reason for this PR

  • Current private methods make it hard to customize
  • We add the flexibility for other implementor to overwrite this method
  • The old version (0.5.3) uses protected, but the newest version changed it to private. I think changing back to protected will make it more backward compatible.

@dmikurube
Copy link
Member

It was protected just because it had another subproject embulk-input-riak_cs, but it has gone because Riak CS is no longer actively developed. Then protected got no longer needed. There is no compatibility issues.

Can you explain more concrete use-cases that "the flexibility for other implementor" is more useful?

If "flexibility" is always more important, we may make everything public, right? But, we usually don't do it indeed because controlling visibility of classes / methods is important to keep things maintainable. We need reasons convincing maintainers to break the visibility control.

@legiangthanh
Copy link
Member Author

In the latest version, almost methods in S3FileInputPlugin.java is private which makes we can not customize the behavior of this class. It means we only use this plugin the way it is implemented without any customizing. For example, In our case, we need to override getCredentialsProvider method to add some validation and reject the unused authentication method.

For backward compatibility, I think we released this plugin in the past, we should keep the contract intact as much as possible because we don't know how others use this plugin.

@dmikurube
Copy link
Member

It means we only use this plugin the way it is implemented without any customizing.

Yes, that is the expected use of Embulk plugins. Embulk plugins do not expect to be used as a library from other things. OSS Embulk plugins do not care about such "backward compatibility". embulk-input-riak_cs was managed in this same repo, and everything was under control. We have never provided the method as a public contract.

I'm asking you to describe the use-case why it is better here to overcome the basic principle that "Embulk plugins do not expect to be used as a library from other things".

@dmikurube
Copy link
Member

Hint:

In our case, we need to override getCredentialsProvider method to add some validation and reject the unused authentication method.

In other words, why don't you contribute that part to this open-sourced embulk-input-s3? Explaining the reason why yon don't contribute it may explain the reason why such customizability would be useful for public users / developers, too.

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

Successfully merging this pull request may close these issues.

2 participants