marshmallow-code / apispec

A pluggable API specification generator. Currently supports the OpenAPI Specification (f.k.a. the Swagger specification)..
https://apispec.readthedocs.io/
MIT License
1.17k stars 177 forks source link

Split out web framework plugins into separate package #302

Closed ergo closed 6 years ago

ergo commented 6 years ago

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 commented 6 years ago

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

lafrech commented 6 years ago

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 commented 6 years ago

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

I also updated 1.0 milestone.

ergo commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Thanks @ergo for tackling this.

Thoughts, open to discussion:

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

ergo commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

How about renaming the package to apispec-web? because

  1. Existing plugin packages don't have "ext"
  2. web is less typing than webframeworks :smirk:
lafrech commented 6 years ago

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 commented 6 years ago

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

ergo commented 6 years ago

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

ergo commented 6 years ago

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

sloria commented 6 years ago

@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 commented 6 years ago

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

sloria commented 6 years ago

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

ergo commented 6 years ago

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 commented 6 years ago

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