architecture: InstallationBasedTask reaches into GitHub details

[AlexanderLanin]

I've just created a GitHub class which internally uses RestApi and GraphQLClient for different functionallity. Naturally the fact which function goes to which API is hidden from users.

Now the problem: InstallationBasedTask uses rest_api and graphql_api directly. It does not use GitHubProvider at all.

I assume this is historical and not intentional?

[netomi]

The GitHubProvider was intended as an abstraction layer for the model classes. At some point there was the idea to support more than GitHub, so you would have different providers.

For the InstallationBasedTask this abstraction does not really make sense imho, so the task uses what it needs,

[AlexanderLanin]

Ah I see.

IMHO it nevertheless makes sense to hide rest vs graphql information within a GitHubProvider abstraction layer. Even with only one provider, changing some functionality from rest to graphql would otherwise require adaptions within the model and within the InstallationBasedTask.

Proposed change in #605: have a pr.create_or_update_comment function (not included in PR, so far only preparatory refactoring). That function uses graphql to retrieve comments, and rest to write a comment. That's why it doesn't fit into RestApi or GraphQLClient class. It needs to be on the GitHubProvider level.

[AlexanderLanin]

Maybe to phrase it differently: making GitHubProvider exchangeable with a generic Provider would be pointless, as there is only GitHub. But hiding how it works internally, seems like a good boundary?

[netomi]

yeah makes sense