abstracted doc ids using enums

[ekohilas]

I removed the magic doc id numbers and moved them to an enum so that they could be better documented to understand what the code is doing.

If there are better names for the doc ids please feel free to change them

[ekohilas]

I've just found the single use of from_query_id with the same id. Should both of these be put under the same function which abstracts which of the two fields it uses? Do you expect this to stay as a single edge case?

[madsmtm]

The two uses of the same query_id is in fetch_thread_images, which should probably be refactored in a way that this doesn't happen, so I expect that to stay as a single edge case.

In general, thanks for the PR, but I'm not a big fan of the change. I recently removed the ReqUrl enum, see #433, because it really just added extra indirection. I'll keep the PR open for now, so that you, or someone else, may change my mind 😉.

[ekohilas]

I can understand the reasoning for removing the ReqUrl enum, given the extra indirection hides the actual url which gives some context, however in this case the indirection serves useful in giving meaning to magic numbers that newcomers to the code won't understand without implicit knowledge (that isn't documented anywhere).

Since you mention it, maybe I'll make some more commits refactoring the case mentioned for this pr to happen.

However I remember that there was a table of ids before, which now seems to have disappeared and thus the number of id's reduced. Am I remembering this correctly? And if so, maybe the real refactoring work should be done on removing the dependency on these ids if possible?

[madsmtm]

I remember that there was a table of ids before

Hmm, don't recall that, but I may be mistaken.

And true, having the ids in an enum does communicate the meaning of them more clearly, but it also means that you now have to look elsewhere to see the actual ID. And if you want to update it (which does happen quite frequently), you'll now have to go to a separate file, verify that the ID is only used the place you're editing, and then change it.

If the issue is that they're hard to understand, then indirection isn't the solution IMHO - a comment would be better, or perhaps some focused documentation in _graphql.from_doc_id?