-
-
Notifications
You must be signed in to change notification settings - Fork 139
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
base: main
Are you sure you want to change the base?
Conversation
(at least youtube3 builds, the others get generated successfully)
(at least youtube3 builds, the others get generated successfully)
all apis libs build now
(this test currently fails and should be fixed at some point)
This reverts commit b9b8cec.
(copied version from `mako-render`)
This is needed since some builds do not remove old files and then cargo doesn't know which one to use. (for example GitHub-Actions)
I think I got most of the issues fixed. Currently I'm stumbling over a build error in This is the 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. These issues currently causes the CLIs not to build. |
It's interesting you are encountering the issue with Regarding the API change and the CLI compatibility, indeed that would mean one would have to provide a Thanks again for your great work here, it's awesome to see this improved. |
I already implemented the |
And I thought with the API providing
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 |
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.") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.
…name overlaps might happen
@OMGeeky do you want help with this? |
Honestly I have not done anything here in a while now. I've hit a dead end and I didn't really like my workflow with mako etc. so it's pretty far but incomplete. So if you wanna help completing this I'm all for it, maybe we can get this finished. |
Sure if you want to drive it to completion, I'm hoping to have some time coming up in a few weeks and this feels like a good place to spend some of that. Or we could wait a while and see how Google's effort shapes up. Either way I think it's a good learning experience for me to help with completing this so either way I don't mind driving it to completion if you want to. I've never worked with generated code I didn't generate myself (amateur hour lol). It was to generate latex from python code. |
Hi @OMGeeky I've been following Google's development over the last few days and it seems like they have at least one person on this full time. So I think maybe better we just wait. Thank you for the work you've done so far. I think anything we do on this would just be "for fun". So probably better just to keep the existing functionality working until Google is done. |
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
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.