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

Add support for defining filesystem plugins in config #42

Merged
merged 3 commits into from
Feb 11, 2015

Conversation

yellow1912
Copy link
Contributor

It makes sense to define the list plugins we want to use for the
filesystem directly in the config. This way we don't have to mess
with the service's definition.

It makes sense to define the list plugins we want to use for the
filesystem directly in the config. This way we don't have to mess
with the service's definition.
@yellow1912
Copy link
Contributor Author

I will add this to the docs if the PR is accepted :)

@yellow1912
Copy link
Contributor Author

@sheeep any thought on this?

@sheeep
Copy link
Contributor

sheeep commented Jan 29, 2015

This is a great idea! Should we get rid of the configuration way of defining plugins (oneup_flysystem.plugin tag)? What do you think? Focus on a single method, or let both possibilities in this bundle?

We should probably add the plugins provided by Flysystem itself as services.

@yellow1912
Copy link
Contributor Author

If you are not worried about Backward Compatibility I say we provide only 1 single method to make things simple

@sheeep
Copy link
Contributor

sheeep commented Jan 29, 2015

As we're bumping the major version anyways, I'd suggest to stick to a single method.

@yellow1912
Copy link
Contributor Author

What is the next step? Do you want something else to be added/removed before we go ahead with this PR? :)

I removed the plugin compiler pass in favour of the new service-way of attaching plugins to your filesystem.
Also checked, that the test suite runs without errors and documented it clearly.
@sheeep
Copy link
Contributor

sheeep commented Feb 4, 2015

Take a look at your repository. I created a pull request which removes the old method of attaching plugins and defines the ones that are already provided by the flysystem library.

Another thing to discuss. Removing the PluginPass also removes the support for attaching a plugin "globally" which means: to every defined filesystem. Should we re-add this feature?

I've thought about something like:

oneup_flysystem:
    plugins:
        - global_plugin
    filesystems:
        my_filesystem:
            plugins:
                - local_plugin

Whereas global_plugin is available for every configured filesystem and local_plugin only for my_filesystem.

What do you think?

@yellow1912
Copy link
Contributor Author

Definitely makes sense to me :)

@sheeep
Copy link
Contributor

sheeep commented Feb 4, 2015

Definitely makes sense to me :)

Very well. I'll create another pull request to your repository asap. :)

Removed the old method to attach plugins.
sheeep pushed a commit that referenced this pull request Feb 11, 2015
Add support for defining filesystem plugins in config
@sheeep sheeep merged commit f329bda into 1up-lab:master Feb 11, 2015
@sheeep sheeep mentioned this pull request Feb 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants