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

Why does Types.rs exist? #109

Closed
erichCompSci opened this issue Nov 10, 2021 · 4 comments
Closed

Why does Types.rs exist? #109

erichCompSci opened this issue Nov 10, 2021 · 4 comments

Comments

@erichCompSci
Copy link
Contributor

Hello,

As I was working on this repo to try and get the loginlink working, I realized that every time I ran the generation code on master the code generation breaks because of updates. I didn't want to push my link to a previous tagged commit (i.e 13-rc3), so I started to look into why code generation was breaking.

The first issue was relatively easy to fix. Quote is a dependency of a type that it implicitly generates and therefore falls into a corner case that incorrectly adds itself as a resource use to its own generated file.

But the second issue is a little bit more intense. Some dependent types that have more than one dependency are skipped to generate. This makes a little bit of sense as they are found inside of a "generated" file so to speak and a naive solution would allow you to have two of the same "type" published from two or more different source directories.

The current solution, seems to be to hand code these types into resources Types.rs. This seems like a non-tenable solution that will cause the generated code to continue to break into the future. I don't like out of date API's especially when this particular version of the OpenAPI is the "stable one" according to git.

So, what if we modified the generated main file to either a) add on these extra files onto the end of the objects metadata field (this would generate a unique file for each, but would require some refactoring because the objects are borrowed immutable in a number of places) or b) create a new category in metadata that runs after the objects and essentially does the same thing as a) but sidesteps the problem of having to add on to a container that is running inside an iterator loop.

Then you would need to remove some of the hand written types that no longer need to exist.

Thoughts?

@erichCompSci
Copy link
Contributor Author

Following up on this, I implemented b. It required a lot of things to change. There were several bugs in the code generation portion that were being papered over by hacks in the mapping portion and hand written code. I fixed them and started removing the handwritten code, particularly in types.rs.

So far, the current API is compiling again. I've also refactored the main generation code so that it's more readable (more functions broken up into logical groupings rather than one giant function with several loops.)

The code is compiling on all 5 targets and has been tested (though the tests don't have very much coverage). I can make a pull request if you want, but I'll wait for a response.

The biggest change I had to make was to make "nullable" objects in the API, Box<Option<>> rather than just Option<>. This is because some of them are circular dependencies (the one I know about is the api_errors and the payment_intent structs). An Option<> doesn't allow for this circular dependency to happen in Rust for obvious and well documented reasons.

This means that my update would be a breaking change for sure, but master won't compile with the current code anyway, and my code does, so I think it's a fair trade. Let me know if you want the code or if you'll pass.

All the best, thanks for the work with the async code, it's really great.

@arlyon
Copy link
Owner

arlyon commented Nov 24, 2021

Hi there, sorry I have been away recently.

To give some more context, types.rs is around simply because that's how it was handled in previous library pre-fork. I have been meaning to have the generator automatically pick it up but haven't gotten round to it.

I am very much for these changes (thanks!!!!) and very keen to see a PR / get them merged. I have been deliberately holding off of the next release until I could fix this so this is great news.

As for refactoring the generation code: once again, thanks, that has been improved compared to the previous version but still needs some modernising.

Looking forwards to seeing the PR up.

Cheers, Alex

@arlyon
Copy link
Owner

arlyon commented Nov 24, 2021

(also, it's been annoying me that the automated generate PR fails every week so it'll be nice to not have to keep closing it 🎉 )

@erichCompSci
Copy link
Contributor Author

I opened a pull request for this issue. Since it's larger and #108 depends on it, I'll wait to pull request that feature after this one is accepted.

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

No branches or pull requests

2 participants