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

let Enums be actual rust Enums & split into modules #497

Draft
wants to merge 70 commits into
base: main
Choose a base branch
from

Conversation

OMGeeky
Copy link
Contributor

@OMGeeky OMGeeky commented May 13, 2024

I found it quite impractical to always look up what options are available for each enum & pass the as strings, so I created this PR which turns the enums provided by the API into enums in Rust.
During this whole thing I had some problems with the file being quite large and my IDE struggling to not lag all the time, so I separated the individual parts into modules, which were currently only separated by a comment.

With this I can for example use

VideoStatusPrivacyStatusEnum::Private

instead of
private
for the YouTube-API and actually get compiler errors when i misspelled something.

Note that I appended Enum to each enum name to reduce collisions with other existing types etc.

I am open to Ideas and willing to put some time into this if any changes need to be made.

Copy link
Contributor Author

@OMGeeky OMGeeky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am currently a bit struggling with some cases where in most cases the stuff passed into schema_markers is a DictObject most of the time (during normal build every time currently) and a normal dict (during tests).
Is there a particular reason to not use the DictObject syntax and just access its values like in a normal dict?

I feel like this might just be a cosmetic/style issue but I am willing to adapt if you have some Idea how to get around this.

@Byron
Copy link
Owner

Byron commented May 17, 2024

Is there a particular reason to not use the DictObject syntax and just access its values like in a normal dict?

I think it's an oversight that a normal dict is passed during tests, maybe a conversion is missing?

I feel like this might just be a cosmetic/style issue but I am willing to adapt if you have some Idea how to get around this.

These DictObjects are indeed just made for convenience, and I thought they make the code more readable. It's probably easiest to try to make sure everything is a DictObject as expected.

@OMGeeky
Copy link
Contributor Author

OMGeeky commented May 17, 2024

The problem is I couldn't figure out how to reference the module from the tests. The DictObject is defined inside the mako_renderer which isn't really a normal python module (and my IDE normally doesn't even detect it as python code if I don't tell it).

@Byron
Copy link
Owner

Byron commented May 17, 2024

I see. Given how 'old-stable' this code is, I think it's fine the DictObject definition into the test code and use it there. That should be the easiest and most convenient.

@OMGeeky
Copy link
Contributor Author

OMGeeky commented May 18, 2024

I think I got most of the issues fixed. Currently I'm stumbling over a build error in bigtableadmin2 where there is a struct containing itself, which isn't allowed directly since it cannot calculate its size. The nested struct needs to be a reference or Box or something like that.

This is the error:

    Checking google-bigtableadmin2 v5.0.5+20240331 (/home/omgeeky/Rust/google_drive/google-apis-rs/gen/bigtableadmin2)
error[E0072]: recursive types `GoogleBigtableAdminV2TypeAggregate` and `Type` have infinite size
    --> src/api.rs:1067:1
     |
1067 | pub struct GoogleBigtableAdminV2TypeAggregate {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
1071 |     pub input_type: Option<Type>,
     |                            ---- recursive without indirection
...
1967 | pub struct Type {
     | ^^^^^^^^^^^^^^^
...
1971 |     pub aggregate_type: Option<GoogleBigtableAdminV2TypeAggregate>,
     |                                ---------------------------------- recursive without indirection
     |
help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to break the cycle
     |
1071 ~     pub input_type: Option<Box<Type>>,
1072 |     /// Output only. Type that holds the internal accumulator state for the `Aggregate`. This is a function of the `input_type` and `aggregator` chosen, and will always specify a full encoding.
   ...
1970 |     
1971 ~     pub aggregate_type: Option<Box<GoogleBigtableAdminV2TypeAggregate>>,
     |

For more information about this error, try `rustc --explain E0072`.
error: could not compile `google-bigtableadmin2` (lib) due to 1 previous error

and the same one is also happening on the main branch, so its not from my changes.

One thing that is from my changes is that the values can no longer just be passed in as a string. They need to be converted to an enum if they really need to be strings or just be enums.
That is a breaking change, but in my opinion its worth it and I got no good Idea how to not create a breaking change at that specific point. The only way I could think of would be to make the method definitions generic accepting an Into<Enum> (or TryInto<Enum> since not all string values can be converted and would get the program to panic if invalid), but I can't think of a good way to change the method signature so it works.

These issues currently causes the CLIs not to build.

@Byron
Copy link
Owner

Byron commented May 18, 2024

It's interesting you are encountering the issue with bigtableadmin2 as failing APIs are usually ignored entirely. Since it's on main, and if it prevents me from releasing the crates, I'd just put it onto the ignore-list.

Regarding the API change and the CLI compatibility, indeed that would mean one would have to provide a TryInto<Enum> generic there and an implementation to convert from &str to Enum fallibly. With that, the CLI should work again, and user-code would still work as well.

Thanks again for your great work here, it's awesome to see this improved.

@OMGeeky
Copy link
Contributor Author

OMGeeky commented May 18, 2024

I already implemented the TryInto for &str but I'm struggling with changing the generating code in the CLI where I have to use it.
One problem I encountered is that not every enum has a default value. I might have an idea how to get around some of the problems, but I don't really fully get others.

@Byron
Copy link
Owner

Byron commented May 18, 2024

And I thought with the API providing TryInto<Enum> that the CLI code wouldn't have to change at all.

One problem I encountered is that not every enum has a default value.

That's interesting, as I don't even know how this works in the existing code, or if it relies on a default value at all. Maybe the implementation on main just makes something up or has some heuristic?

call = call.${mangle_ident(setter_fn_name(p))}(&value);
} else {
/*value was empty but has no default*/
todo!("I don't know what would be best in this situation.")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the value provided was either empty or invalid and I'm not sure what to do in this case.
Should we just ignore this value and move on and see if it throws an error when executing the call?
Maybe output a warning?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's possible to make the call anyway, the CLI could probably try it and maybe even emit a warning like you suggested.
Admittedly I don't fully understand this yet, but trying to be lenient for the CLI seems like the right choice here.

OMGeeky added 7 commits May 18, 2024 19:15
it was causing some issues and I want to focus on other things first.
if we get enum parameters that are required, but either missing without default or invalid, it panics currently. Maybe that can be improved if needed.
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.

2 participants