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

Setting build_only_models to 'true' fails to default with_converter to 'false' #206

Closed
point-source opened this issue Aug 10, 2021 · 11 comments · Fixed by #222 or #230
Closed

Setting build_only_models to 'true' fails to default with_converter to 'false' #206

point-source opened this issue Aug 10, 2021 · 11 comments · Fixed by #222 or #230
Labels
bug Something isn't working

Comments

@point-source
Copy link

By default, with_converter is (correctly) set to true. However, when build_only_models is set to true, the chopper converter is likely not needed but is generated anyway. This results in an error since this line fails to find a chopper.JsonConverter to extend from:

class JsonSerializableConverter extends chopper.JsonConverter {

We should either generate the chopper converter or automatically default with_converter to false when build_only_models is true and with_converter has not been explicitly set by the user.

@Vovanella95
Copy link
Collaborator

Hi @point-source

Good point. I will fix it

@Vovanella95
Copy link
Collaborator

Hi again @point-source ,

We decided to rid of build_only_models parameter. If needed to build only models - just remove them from swagger file.

@point-source
Copy link
Author

Not sure I follow. So if I need the models built but no chopper client, I should be removing what exactly?

@Vovanella95
Copy link
Collaborator

@point-source , You can remove paths from Swagger file.

Also If you really don't want to use Chopper, you can put swaggers to separate lib, generate everything, but use only models. Is it OK?

@Vovanella95
Copy link
Collaborator

@point-source

Specially for you, is this feature really important?

@fryette
Copy link
Contributor

fryette commented Aug 26, 2021

@point-source
We would like to focus on an end-to-end solution when you grab swagger and then make a request.

You have 3 options

  1. Add chopper to the dev-dependencies and use only models
  2. Use swagger-codegen lib
  3. Fork project

@point-source
Copy link
Author

Understood. I suspect this will be fine but cannot test it today. I have one project that was making use of that flag and I will test asap and let you know if there's any issues. Thanks for the quick support and communication!

@Vovanella95
Copy link
Collaborator

All at all let's return this feature back and fix this issue. I will handle it tomorrow.

C.C. @point-source

@Vovanella95 Vovanella95 reopened this Aug 26, 2021
@point-source
Copy link
Author

While I truly appreciate your willingness to reverse a decision like this so quickly based (at least on part) on my input, I would hate for you to waste any of your time. If it's alright with you, I would prefer to test without the use of the flag to see if it's even needed before you put additional work in.

I have no problem adapting if it helps reduce surface area and improve stability/maintainability of this project. I'm using this tool across many projects and only one appears to need this flag.

Additionally, if generating the full chopper client doesn't break anything, I can just let it do that and dart's tree shaking should pull out the excess code at compile time anyway.

I will test as soon as I can. Thanks!

@fryette
Copy link
Contributor

fryette commented Aug 26, 2021

@point-source Thanks!
Would be nice if you can share results related to the tree-shaking.

We also decided to see how with less pain support that features (in the last few months @Vovanella95 did a great job for rewriting some part of the library).

@fryette fryette added the bug Something isn't working label Aug 26, 2021
This was referenced Aug 27, 2021
@Vovanella95
Copy link
Collaborator

@point-source
I've fixed your issue. Please try it on 2.1.0-prerelease.9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants