Update to Hyper 1 #524

IvanUkhov · 2024-10-02T13:26:15Z

Closes #459.

IvanUkhov · 2024-10-03T14:01:02Z

@Byron, just wanted to get your early feedback. I have managed to make the API crates compile. At least, it passes for:

make storage1-clean storage1 storage1-cargo ARGS=test

It remains to update the CLI template.

Is this something that would would be willing to accept, or is it not the direction you want to take?

Byron · 2024-10-03T14:24:35Z

Thank you, the work looks very promising and I'd be happy if we could see it through.

My recommendation is to get CI green first - the issues there seem to be unrelated to the CLI for now.

And regarding the CLI, I believe to remember that they didn't compile before your change either, it degenerated (maybe CI isn't actually testing these), and getting them to work again would be greatly appreciated - maybe there are still useful to some.

IvanUkhov · 2024-10-03T14:52:49Z

Now the CI fails at the CLIs. I will then tackle them next.

IvanUkhov · 2024-10-04T14:34:06Z

I added storage1 to the CI list to show the error I am facing (was passing for the other ones on the list). Please take a look. Looks like there is some confusion regarding type and format. An argument can be of type string and format int64, and the function is generated to take an integer while the CLI tries to pass a string. Do you have any ideas regarding what might be going on and how to address that?

Byron

Thanks a lot, this is great work and I think it is very, very promising!

That the CLI fails to compile for storage1 wouldn't even be a hold-up for this PR - the CLI was fully broken before this PR, and now it's only partially out of sync with the API with certain APIs even , I'd call that a huge improvement already.

With that said, I also don't know why it gets its typing wrong, maybe it's a split-brain issue somewhere?

However, there is one question I had when looking at a removal.
Further, did you managed to run cargo check on all the APIs? Do they still work?

If so, I think we can try to get this PR merged and a new major release created.

Thanks again!

Byron · 2024-10-04T18:27:06Z

src/generator/templates/api/lib/mbuild.mako

@@ -703,17 +698,6 @@ else {
            request_value_reader.seek(io::SeekFrom::Start(0)).unwrap();
            % endif
            let mut req_result = {
-            % if resumable_media_param:


Would this not be missing somewhere?

I was planning to ask you about this. It was getting in the way, but I could not find how it was actually used. It looks like dead code. The way I see it is that should_ask_dlg_for_url is always false. Or what is going on here?

Right! I checked and the variable was false from the very beginning, so it could never have worked anyway.

Also something I just noticed is the 'assignment as side-effect of if-condition' dark magic that is performed here, and it's incredible that I was the author of this 😅.

In any case, I agree that under the circumstances, it should definitely be removed.

Byron

Thanks a lot for following up!

From my point of view this would be ready to merge, unless of course you'd like to perform more testing or try to figure out how to fix that last remaining CLI generation issue.

It's entirely up to you.

Byron · 2024-10-06T06:09:38Z

src/generator/templates/api/lib/mbuild.mako

@@ -703,17 +698,6 @@ else {
            request_value_reader.seek(io::SeekFrom::Start(0)).unwrap();
            % endif
            let mut req_result = {
-            % if resumable_media_param:


Right! I checked and the variable was false from the very beginning, so it could never have worked anyway.

Also something I just noticed is the 'assignment as side-effect of if-condition' dark magic that is performed here, and it's incredible that I was the author of this 😅.

In any case, I agree that under the circumstances, it should definitely be removed.

IvanUkhov · 2024-10-06T09:57:19Z

I have plugged in a few Python linters and cleaned up trailing whitespace and such. Also, clarified the CI workflow.

I will try to regenerate all APIs and run the checks with the tests now.

IvanUkhov · 2024-10-06T16:22:17Z

OK, I am able to generate all APIs and the following succeeds:

 make cargo-api ARGS=check

Byron

Thanks a lot for the exhaustive update and modernisation!

It's great that CI is now more granular, too. Maybe it could be split between API and CLI as well so it's more clear that only CI currently fails, and nothing else.

Maybe with all the time spent in the code-base, it will soon become more obvious why some CLIs don't yet build. It seemed like it must be a small thing… .

Byron · 2024-10-06T18:10:02Z

etc/api/type-api.yaml

@@ -25,9 +25,3 @@ make:
 cargo:
  keywords: [protocol, web, api]
  doc_base_url: https://docs.rs
-  dependencies:


It's a great idea to bring it all into the generator file.

src/generator/templates/deps.mako

IvanUkhov added 2 commits October 2, 2024 15:24

Update google-apis-common

926ad7b

Update google-clis-common

b70df88

IvanUkhov changed the title ~~Update all dependencies~~ Update to Hyper 1 Oct 2, 2024

IvanUkhov added 13 commits October 2, 2024 15:38

Export Body and Response

98c432c

Remove get_body_as_string

a654f2c

Introduce to_body

8835901

Introduce Client

82a00cc

Introduce to_response

5a494f3

Remove to_json and add to_string

8978b94

Remove the json alias

70bfc8f

Introduce Client and Connection

6d3710f

Run cargo fmt upon generation

07a4d62

Make the API code compile

b6fac32

Make the API documentation compile

a72a918

Switch to the 2021 edition

c171cd6

Rename Connection to Connector

c67b9d3

IvanUkhov added 3 commits October 3, 2024 16:36

Address the lints

4bbf1ee

Update the tests

c4fc156

Do not try to format the CLIs

1052f0f

IvanUkhov added 3 commits October 3, 2024 17:11

Start to update the CLIs

93a30e0

Create a separate client for yup-oauth2

d15dcfd

Add storage1 to the CI

e05dea3

Byron reviewed Oct 4, 2024

View reviewed changes

IvanUkhov added 3 commits October 5, 2024 22:03

Sort out the dependencies

f62b45e

Address a few lints

35d780e

Adjust a docstring

13ae02e

Fix the usage of FieldMask

9bb8a42

Byron reviewed Oct 6, 2024

View reviewed changes

Sort out new lines and trailing whitespace

8e80440

IvanUkhov marked this pull request as ready for review October 6, 2024 17:01

Byron approved these changes Oct 6, 2024

View reviewed changes

Make clear why CLIs are not being formatted

d6bd590

Byron merged commit 450748c into Byron:main Oct 6, 2024
4 of 5 checks passed

IvanUkhov deleted the yup-oauth2 branch October 7, 2024 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to Hyper 1 #524

Update to Hyper 1 #524

IvanUkhov commented Oct 2, 2024 •

edited

Loading

IvanUkhov commented Oct 3, 2024

Byron commented Oct 3, 2024

IvanUkhov commented Oct 3, 2024

IvanUkhov commented Oct 4, 2024

Byron left a comment

Byron Oct 4, 2024

IvanUkhov Oct 5, 2024 •

edited

Loading

Byron Oct 6, 2024

Byron left a comment

Byron Oct 6, 2024

IvanUkhov commented Oct 6, 2024

IvanUkhov commented Oct 6, 2024

Byron left a comment •

edited

Loading

Byron Oct 6, 2024

Update to Hyper 1 #524

Update to Hyper 1 #524

Conversation

IvanUkhov commented Oct 2, 2024 • edited Loading

IvanUkhov commented Oct 3, 2024

Byron commented Oct 3, 2024

IvanUkhov commented Oct 3, 2024

IvanUkhov commented Oct 4, 2024

Byron left a comment

Choose a reason for hiding this comment

Byron Oct 4, 2024

Choose a reason for hiding this comment

IvanUkhov Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

Byron Oct 6, 2024

Choose a reason for hiding this comment

Byron left a comment

Choose a reason for hiding this comment

Byron Oct 6, 2024

Choose a reason for hiding this comment

IvanUkhov commented Oct 6, 2024

IvanUkhov commented Oct 6, 2024

Byron left a comment • edited Loading

Choose a reason for hiding this comment

Byron Oct 6, 2024

Choose a reason for hiding this comment

IvanUkhov commented Oct 2, 2024 •

edited

Loading

IvanUkhov Oct 5, 2024 •

edited

Loading

Byron left a comment •

edited

Loading