search

Byron / google-apis-rs

A binding and CLI generator for all Google APIs
http://byron.github.io/google-apis-rs
Other
1.01k stars 131 forks source link

Fix relative links #421

Closed samoylovfp closed 1 year ago

samoylovfp commented 1 year ago

Fixes #418

Recommended to review commit-by-commit

samoylovfp commented 1 year ago

Planning on taking another stab at it next weekend or sooner

samoylovfp commented 1 year ago

Apologies, didn't find the time over the previous weekend.

Regarding the rust_doc_sanitize call missing on method argument descriptions.

I did this rough test, checking if the guard condition ever evaluates true in this position

    if has_markdown_codeblock_with_indentation(s):
        raise ValueError("Hello!")l

It doesn't, which indicates that the call is not necessary there now;

However I entertained the thought that Google might add a code block to this description later and tried to understand what the preprocessor is actually doing there to gauge if we can add it there for future-proofing. I commented out the useful part of the preprocessor to see which parts of the documentation is affected, checked the first generated file that was different and saw the result that is currently published here https://docs.rs/google-adexchangebuyer2_v2_beta1/latest/google_adexchangebuyer2_v2_beta1/api/struct.AccountClientInvitationGetCall.html#method.delegate The result does not look intentional. I assumed that the intent was to generate a markdown block something like

It should be used to handle progress information, and to implement a certain level of resilience.

Where the code block is used for text emphasis.

Reading the commit message https://github.com/Byron/google-apis-rs/commit/8cb73d66c245f0f15bbe5c697f92c0351c41fd38 there seems to have been an assumption that rustdoc can parse CommonMark which currently does not hold.

I feel like this is a separate issue though.

Conclusion

I still think that the preprocessor call is warranted there and will teach it handling the relative links.

Byron commented 1 year ago

Thanks for the update! It looks like CI is broken similarly on main, and it's an error in the makefile which surprises me - I don't think that was touched in a while and it did work before apparently (just rerunning main with an unrelated change triggered it). That seems to be something new, maybe with make itself which is not encouraging.

Sorry for that - at some point I might just have to archive this beast or… redo the build automation in Rust.

samoylovfp commented 1 year ago

Oops forgot to convert this to draft. It is still not complete, so no worries about the pipeline. And warm support to the idea of redoing the build process and everything else regarding this project in rust, I feel like it will lower the barrier of entry for people who are not familiar with python and python templating

Byron commented 1 year ago

I feel like it will lower the barrier of entry for people who are not familiar with python and python templating

That's true - even though python templating would remain for generating code, but it wouldn't be used to generate makefiles anymore. Instead, there would be some sort of entry point written in Rust to perform common operations that include running the generator.

The reason Make was chosen in the first place was its natural ability to only run the generator if needed, but given how fast it is I don't even think such an optimisation is necessary.

Regarding the current make issue, I remember that previously it would always show a warning for some include not working, the first time, but then it would generate it and re-run itself automatically in some sort of self-healing process.

After short investigation it turns out to still be the case, it's just the last warning we see, it's just that codecov installation is broken now.

After removing this, pyyaml installation breaks even though I don't know if that was related - clearing out the virtualenv and retrying actually worked. That was easy ;). So turns out that rewriting these portions in Rust wouldn't have fixed the issue at all as really python is the problem, and redoing code generation is definitely out of scope.

So I guess it's keeping this old machine running until Google finally steps up to do it right.

samoylovfp commented 1 year ago

Sorry for the prolonged silence, finally found some time and energy to finish this.

Some urls are now fixed, however for example the Firebase documentation contains relative links starting with "../../" and documentationLink without path, so resulting links are still broken, but at least some relative links are now working

Byron commented 1 year ago

Thanks a lot, it's much appreciated!

I was about to test it locally be regenerating and cargo checking all APIs, but realized that this is a force-pushed commit that doesn't contain my ones which made a bunch of upgrades to dependencies as well. Those seem entirely lost now.

Would you mind reconciling both? My changes can be found in this branch, and I think most of them will cherry-pick in-order just fine.

Please let me know. Thank you.

samoylovfp commented 1 year ago

Oops! I just rebased onto the latest main and missed the part where you added something, will fix.

samoylovfp commented 1 year ago

I apologize for the confusion caused by the force push, but I think your latest commits to your main branch are now included in this branch and this pull request. I force pushed because I wanted to clean up the WIP commits and didn't think how it will look on this page.

One more confusing thing is that there are now two branches called fix-relative-links, one belonging to your repository and one belonging to my fork of it. I failed to find how to better see the branch relations on github, but locally I can view them using the "git tree" addon for vscodium and it looks like this, the "origin" is this repository and "fork" is my fork of this repository.

image

Are you OK with the current state of this PR or do you want me to re-create it? I can also revert to the previous state if you're OK with having WIP commits there.

Byron commented 1 year ago

No worries at all :).

I can also revert to the previous state if you're OK with having WIP commits there.

If the previous state contains all your changes and all my changes, or makes it easier to merge them together, I'd definitely prefer that and happily take the WIP commits and everything that comes with it. In the end, whatever is easier for you should be done, whether that's cherry-picking or merging in a pre-squash history or something else.

samoylovfp commented 1 year ago

I believe then the current PR is ready to be merged as is

Byron commented 1 year ago

Some changes seem to be there, others like this one don't seem to be present, which makes me fail to understand what is what. Since what is here is definitely not what would be there if both version were reconciled, it seems like the better idea to remove the squash and merge both histories, fixing conflicts as they arise. Feel free to choose your version over mine - my goal was just to get it to compile, your goal was of higher value. Thant would be great, thanks for your help if you can extend it knowing time is scarce.

samoylovfp commented 1 year ago

How does this look? It is now just contains two additional commits on top of https://github.com/Byron/google-apis-rs/tree/fix-relative-links

Byron commented 1 year ago

Sorry, I accidentally force-pushed into this PR, even though it should have gone into #429 . Feel free to force-push over it to restore the previous state for posterity.

Besides that, thanks for trying and your continued engagement, but I decided to go with my branch as base along with manually ported changes from single-commit state here.

A new release that incorporates the fixes should happen in the course of the year, along with an API update.

This PR will be closed in favor of #429.

samoylovfp commented 1 year ago

I am still confused how github tracks the source branch of a PR. Thank you for finishing this!