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

Some links in the documentation published to docs.rs are incorrect #418

Closed samoylovfp closed 1 year ago

samoylovfp commented 1 year ago

A concrete example is on this page https://docs.rs/google-sheets4/5.0.2+20230119/google_sheets4/api/struct.SpreadsheetMethods.html#method.values_append Clicking on guide opens https://docs.rs/sheets/api/guides/values#appending_values which does not exist, instead of https://developers.google.com/sheets/api/guides/values#appending_values Same issue is other links.

Byron commented 1 year ago

Thanks for letting me know! It looks like the issue happens due to an invalid base URL or due to the usage of relative URLs. Both should be a relatively straightforward fix, for which a contribution will be much appreciated.

denolia commented 1 year ago

Hey @Byron we've been investigating the fix together with @samoylovfp and here is our current understanding:

  1. As you mentioned, indeed, /etc/* files are generated on the basis of google api and we shouldn't touch them in scope of this fix. Theoretically, we could postprocess the jsons after downloading them, for example here, but this python script doesn't seem to be a good place for the fix.
  2. We need to fix the code that is processing MD documentation lines and links inside it. There are two cases:
    1. For the docs in lib.rs, it seems to be possible to do the fix in lib.mako. The attempt is made here, although it doesn't work. We'll continue investigation.
    2. For the docs in api.rs, doc strings are processed by python script here which runs the rust parser in case if there are code blocks. If we understand the data flow correctly, we should add the doc link preprocessor here as well: either to rust or to python.

Please correct us if we are on the wrong path.

Byron commented 1 year ago

Thanks for sharing your analysis, it's much appreciated.

I second the opinion in 1.

Regarding 2.1, I would have thought this is the spot. In the implementation it appears you are turning relative URLs into absolute ones, but would have expected accessing the documentationLink to get the documentation root. Maybe the rest of the URL also needs adjustment in that case. In any case, the entire API document is available in the globals of the Mako source (I think).

Regarding 2.2, let's keep it as a Plan B if everything else fails. Generally the preprocessor doesn't have any context yet so it's less suited to do that.

I hope that helps.

samoylovfp commented 1 year ago

We need to implement both 2.1 and 2.2 for all of the links to be generated properly. I'll try to see how that can be implemented

Byron commented 1 year ago

We need to implement both 2.1 and 2.2 for all of the links to be generated properly.

Can you elaborate? 2.2 is merely a 'should not be needed' fallback in case 2.1 goes terribly wrong even though it shouldn't. The generator seems to have all the data it needs to create correct documentation links which makes it the best location for a fix.

What am I missing?

samoylovfp commented 1 year ago

Sorry, I misread the original comment, this is not related to the preprocessor, but to api.rs.

If we read the source correctly, the entrypoint into the doc comment processing stack for lib.rs is templates/api/lib.rs.mako and for method builders in app.rs it is templates/api/lib/rbuild.mako

However the latest experiment shows that we have missed something, as the link is "fixed" two times. https://github.com/samoylovfp/google-apis-rs/commit/55a99693415c4e6139e7c359fe3574946c046a39#diff-fed387ffc0d44b87ca96cdef3efd5cdd5da6cdfa0243c42cbde1ea49b58c6af1R6932

I'll continue the investigation and post more once I understand what I am doing

samoylovfp commented 1 year ago

Double "fix" was due to the logic of fixing the url. The source jsons contain both relative and absolute urls. Once the proper parsing had been implemented the issue went away.

My faulty assumption was that documentation for lib.rs might also contain links relative to external sites.

Empty output from grep --include lib.rs "](/" -r . disproves that, so IMO it is enough to fix the app.rs generation.

I tried that in the PR, please check it out. I manually clicked several of newly-fixed links, they seem to work correctly.