Skip to content
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

Hide truncatedresult warnings #2

Closed
lucaswerkmeister opened this issue Feb 6, 2022 · 4 comments
Closed

Hide truncatedresult warnings #2

lucaswerkmeister opened this issue Feb 6, 2022 · 4 comments
Labels
enhancement New feature or request

Comments

@lucaswerkmeister
Copy link
Owner

Part of the purpose of this module is to automatically handle truncated responses, so we should hide this warning if it happens.

@lucaswerkmeister lucaswerkmeister added the enhancement New feature or request label Feb 6, 2022
@lucaswerkmeister
Copy link
Owner Author

Fun fact: errorformat=bc warnings don’t include a code, but they’re always in English and don’t use local interface messages. So I think we can actually reliably detect the warning: in any non-bc errorformat, we can look at the code (truncatedresult); if that’s not present, we check the message against a regex like /^This result was truncated because it would otherwise ?be larger than the limit of .* bytes$/. (Apart from the extra space accidentally added in wikimedia/mediawiki@1c57794 and removed again in wikimedia/mediawiki@f465c7f, the message has been stable since it was introduced in wikimedia/mediawiki@d8a241f.)

@lucaswerkmeister
Copy link
Owner Author

Keep in mind, though, that currently this package nominally supports m3api all the way down to 0.1, which didn’t have warning support yet. So code defensively, or perhaps raise the peer dependency version.

@lucaswerkmeister
Copy link
Owner Author

Also, I’d say that not all the request-making methods should hide this warning. In particular, the “partial” methods don’t handle truncated responses (and don’t include the continuation in the return value, so the caller can’t really handle truncated responses correctly either), so I’d say those should continue to show the warning.

@lucaswerkmeister
Copy link
Owner Author

Keep in mind, though, that currently this package nominally supports m3api all the way down to 0.1, which didn’t have warning support yet. So code defensively, or perhaps raise the peer dependency version.

If lucaswerkmeister/m3api#14 is implemented, we’d presumably want to use that in this package, so in that case we’d raise the peer dependency version anyways. (And I think as long as m3api hasn’t reached 1.0 yet, it’s acceptable for m3api-query to only support recent-ish m3api versions.)

lucaswerkmeister added a commit to lucaswerkmeister/m3api that referenced this issue Feb 19, 2022
Since we’re fully in control of the execution flow within a batch (from
which the user can only escape by throwing in the reducer), we know that
we’ll also retrieve the rest of the truncated result, and the warning
about incomplete data would only apply if the reducer was programmed to
drop the data from the following responses, despite this function being
the one you use when you don’t want that problem. In other words, this
function is the solution to the truncated responses problem (among
others), so we should hide the truncated responses warning.

We can reliably recognize the warning even in errorformat=bc (I think),
because in that case the message is always in English, and does not use
local (on-wiki) messages either: it’s effectively a string hard-coded
into MediaWiki. And within MediaWiki, the message has been very stable:
it accidentally gained an extra space between MediaWiki 1.26 and 1.28,
but otherwise it’s remained constant. So in cases where the warning
doesn’t have a code, we can check the message instead.

Finishes #14; see also lucaswerkmeister/m3api-query#2.
lucaswerkmeister added a commit that referenced this issue Mar 27, 2022
This lets m3api do some of our work for us; it requires the latest m3api
release, but I think it’s acceptable to require recent versions while
m3api is still pre-1.0.

Part of #2, but we should do the same for queryFullPageByTitle(), at
least. Conceptually, queryIncrementalPageByTitle() should also hide
truncatedresult warnings, but I don’t see how to implement that function
using requestAndContinueReducingBatch()… maybe we can just drop that,
possibly along with queryPartialPageByTitle()?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant