bug 1746940: document codeinfo lookup in download API #2827
Conversation
|
@smarnach This isn't urgent. But when you have a free moment, can you review this documentation change? It's related to bug 1746940--specifically the way I overloaded the download API. To make the documentation, you can do: After that, I do Things to review:
|
There was a problem hiding this comment.
I've got a few comments, some of them not related to your changes.
I think we should also explicitly mention that HEAD requests have non-standard semantics for Tecken. The MDN documentation of the HEAD method states
The HTTP
HEADmethod requests the headers that would be returned if theHEADrequest's URL was instead requested with the HTTPGETmethod.
This is not what Tecken is doing. While the documentation describes accurately what Tecken is doing instead, I think we should explicitly acknowledge that this is a bit unusual. We could do that in a separate PR, but it's just adding a single sentence, so we could also do it in this PR.
docs/download.rst
Outdated
| Same as ``HEAD /<DEBUG_FILENAME>/<DEBUG_ID>/<SYMBOL_FILE>``, but this will | ||
| check try symbols and then regular symbols. |
There was a problem hiding this comment.
The documentation further up sounds like using ?try or prefiing with /try are equivalent, stating
Symbols from Try builds is always tried last!
This, on the other hand, sounds like prefixing /try is different from ?try, and try symbols are checked first when using /try.
Somewhat relatedly, the text only mentions to add ?try, while the API reference always uses ?try=1. I would be nice to make that more consistent.
There was a problem hiding this comment.
You're right in that there's a mix of documentation styles as well as different facts in different places and it's inconsistent. I'll do a consistency pass.
docs/download.rst
Outdated
| < Content-Length: 143395908 | ||
| < | ||
| <OUTPUT> | ||
|
|
||
| :reqheader Debug: if ``true``, includes a ``Debug-Time`` header in the response. | ||
|
|
||
| If ``Debug-Time`` is `0.0``: symbol file ends in an unsupported extension |
There was a problem hiding this comment.
Something's wrong with quoting 0.0 here. There's another instance of the same typo further up, also for 0.0. (Not introduced by this change, but we could just as well fix it while we are here.)
There was a problem hiding this comment.
You're right! I should have seen this in the linter output, but missed it.
There was a problem hiding this comment.
Oh, I see. The linter I have doesn't understand the .. http:head:: directive, so it doesn't even look at the content block.
docs/download.rst
Outdated
| Same as ``GET /try/<DEBUG_FILENAME>/<DEBUG_ID>/<SYMBOL_FILE>``, but this | ||
| will look up the debug filename and debug id for a module using the code | ||
| filename and code id. |
There was a problem hiding this comment.
I feel it would be simpler to reference the non-try version of this here.
| Same as ``GET /try/<DEBUG_FILENAME>/<DEBUG_ID>/<SYMBOL_FILE>``, but this | |
| will look up the debug filename and debug id for a module using the code | |
| filename and code id. | |
| Same as ``GET /<CODE_FILENAME>/<CODE_ID>/<SYMBOL_FILE>``, but this will | |
| check try symbols and then regular symbols. |
I've been thinking about this. Earlier, I was of the mind that we should leave things as they are because to change the semantics would require figuring out who's relying on the current behavior, changing their code, changing the behavior, etc and in this situation where we don't have a versioned API, when we change it, we change it for everyone in a way that's not particularly discoverable and it could be a mess. But now I'm thinking we should just fix HEAD to be more correct. It still returns 200/404 based on whether the symbol exists. The change here would be to make the HTTP headers the same as the GET request and I can't think of a way someone would use HEAD such that this change would break things. I wrote up a bug to look at fixing it. https://bugzilla.mozilla.org/show_bug.cgi?id=1861044 |
* fix path info specification to use sphinxcontrib.httpdomain syntax which allows it to be used in references * fix all the language around regular and try build symbols files so it's consistent and less jargon-y * fix documentation around "/try" and "try=1" so it's consistent * remove a lot of redundant documentation by changing how the API docs are structured * add example HTTP requests * include a note for code-info lookup so it's clear it's only helpful for Windows
|
^^^ I started with your comments which were really helpful and then that turned into a minor overhaul of the entire document. This reduces a lot of redundant material, fixes a lot of consistency and clarity problems around different kinds of symbols files and how to get them, fixes path specification to what sphinxcontrib.httpdomain expects so we can do references, clarifies that hex parts need to be hex characters all upper-cased, adds example HTTP requests, adds user-agent everywhere, etc. What do you think about these changes? Is the material easier to follow and reference? |
|
Thank you! |
No description provided.