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

Split out web framework plugins into separate package #302

Closed
ergo opened this issue Oct 5, 2018 · 22 comments

Comments

@ergo
Copy link
Contributor

commented Oct 5, 2018

Hi,

Now that Apispec nears 1.0 release, and given how many issues are open right now I was thinking that maybe web framework plugins should be living in a separate apispec_plugins package.
For bw. compatibility existing namespace apispec.ext.$framework could pull them back from separate package.

This would allow plugin authors to fix bugs in implementations or support newer versions of frameworks without the need to release apispec itself, or even add new plugins.

I will be attending a hackathon soon and we could probably tackle this.

@sloria

This comment has been minimized.

Copy link
Member

commented Oct 6, 2018

I think this could be a good idea. @lafrech Any objections?

@lafrech

This comment has been minimized.

Copy link
Member

commented Oct 6, 2018

Thanks @ergo for offering your help on this.

I find it useful to be able to test all bundled frameworks while modifying apispec, so it is practical to have them in the tests. I also agree that once apispec reaches 1.0, it should not be as necessary.

You know what? Go for it, and let's keep those plugins as dev-requirements and keep the tests for now. Besides, some internal features may be only covered by web framework plugins tests. Not sure but we could check that.

No need to ensure backward compatibility for 1.0 so no need to republish.

I think we can keep marshmallow in.

Should those plugins be hosted in marshmallow-code organization? @sloria, could you create the repos when code is ready?

@lafrech lafrech added this to the 1.0 milestone Oct 6, 2018

@lafrech

This comment has been minimized.

Copy link
Member

commented Oct 6, 2018

I just added plugins label to issues that should be forwarded to plugins bugtrackers.

I also updated 1.0 milestone.

@ergo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

Hi I've added #304 , the repo for the package is https://github.com/ergo/apispec-ext-webframeworks - should I change ownership to marshmallow organisation?
I've tried to do things making the change transparent to end users. There probably is a way to move framework tests to separate package and test both in single run.

@ergo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

I did not want to register any packages yet to solve the chicken/egg problem for test runner since I don't know the conventions Marshmallow project wants to follow here.

@ergo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

I've made invoke test run tests from both packages at once, so testing all the frameworks during apispec development is still possible @lafrech.

@lafrech

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

Thanks @ergo for tackling this.

Thoughts, open to discussion:

  • Shouldn't we make an extension per web framework? (apispec-ext-flask,...) Each extension would hard-depend on its framework, with a minimal compatible framework version (E.g. flask>=0.12).

  • I wouldn't mind backward compatibility. Until 1.0 we can introduce breaking changes. There are breaking changes already, and users are expected to read the changelog and test when upgrading. They'll just have to add e.g. apispec-ext-flask to their dependencies, no big deal.

I suppose marshmallow organization could host apispec-ext-repos but let's wait for feedback from @sloria.

@ergo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

Yup, it would be trivial to make 1 package per framework, still keep tests to work in a single pass, up to you guys. I have no strong opinions here.

@lafrech

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

I like separate packages better but let's wait for @sloria.

I was thinking of keeping all the ext tests, at least temporarily, but I like the invoke trick. That's neat.

I just checked and those tests don't add coverage to apispec core. They really only test their respective frameworks, so they could be dropped soon. And test_ext_combination.py could be removed or abstracted to avoid depending the extensions in the tests.

@sloria

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

On one hand, it's nice to have different packages so they can be released independently. Also, it's better for discoverability, i.e. apispec-tornado is nicer than apispec-ext-webframeworks.

On the other hand, it's much easier to make contributor-facing DX changes across all plugins when they're in a monorepo. I'm thinking of things like pre-commit hooks, TravisCI config, testing practices, dev dependencies, package metadata, etc. This is why I decided to bundle the plugins in apispec.ext in the first place.

I lean toward the monorepo in order to reduce maintenance burden. I think the benefits of having independent release schedules are outweighed by the cost of having to maintain consistency in style and process across multiple repos. Perhaps when we have more than just @lafrech and me maintaining the plugins, we could re-consider the multirepo approach.

@ergo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2018

I agree, so what are the next steps here? In my local tests everything works, apart CI but thats expected until the package gets released.

@lafrech

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

Note that I pushed a change to the tests (including web framework tests) to ensure they don't rely on APISpec internal details. You'll have to rebase to pick those.

Also, I think you can remove the compatibility part, in fact the whole "ext" directory, and just make the web-framework lib a test dependency.

Edit: Of course not the whole "ext" dir. MarshmallowPlugin is kept. I meant all web-frameworks.

@sloria

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

How about renaming the package to apispec-web? because

  1. Existing plugin packages don't have "ext"
  2. web is less typing than webframeworks 😏
@lafrech

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

It's not very explicit.

If we manage to share the responsibility and split into several packages one day, how will we call the packages? apispec-tornado, apispec-flask,... ? Those are a bit more explicit. Although it might not be clear what the difference is between apispec-flask and flask-apispec...

The ext can go, I'd keep framework to be clear what it's about. It should only be until the frameworks are split into individual packages, if that happens.

Whatever you prefer, I don't really mind.

@sloria

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

Fair points. Let's go with apispec-webframeworks.

@ergo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

Hi, I've renamed the package to apispec-webframeworks and rebased on top of latest code.

@ergo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

The framework repo is in https://github.com/ergo/apispec-webframeworks - can I transfer rights to it?

@sloria

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

@ergo You can try transferring it to the marshmallow-code org. IF that doesn't work, transfer it to me (sloria), and I'll transfer it to marshmallow-code.

@ergo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2018

@sloria Request to transfer to you initiated - to transfer to org I would have to have write permissions there it seems.

@sloria

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

Thanks @ergo. I transferred to marshmallow-code: https://github.com/marshmallow-code/apispec-webframeworks

@ergo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2018

Awesome, what are next steps here, can you guys try installing both packages with my pr and see if everything works as it should? I tried running tests locally and everything was fine on my end.

@sloria

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

I'll work on getting apispec-webframeworks published to PyPI when I have some time (hopefully this weekend).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.