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

Encapsulate usages of WebClient #2614

Merged
merged 1 commit into from
Dec 26, 2018

Conversation

HebaruSan
Copy link
Member

Motivation

We currently have some network problems that hypothetically might be helped by transitioning from WebClient to some other library for network access (e.g., #2546), but currently WebClient code is spread through several places in the code, making it difficult to refactor cleanly.

Changes

The Netkan source GitHubApi now accepts an IHttpService parameter as the other Netkan sources do and uses it to access the GitHub API instead of making its own WebClient. This allows tests of this class to "mock" network accesses (though no test does this yet).

All other places that were previously using WebClient directly now call one of two central access points: Net.DownloadText or NetAsyncDownloader. This will make it easier to transition all of this code to some other underlying library in the future if needed.

@HebaruSan HebaruSan added Pull request Netkan Issues affecting the netkan data Network Issues affecting internet connections of CKAN Core (ckan.dll) Issues affecting the core part of CKAN labels Dec 17, 2018
@politas politas merged commit 0febe3d into KSP-CKAN:master Dec 26, 2018
politas added a commit that referenced this pull request Dec 26, 2018
@HebaruSan HebaruSan deleted the fix/encap-webclient branch December 26, 2018 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core (ckan.dll) Issues affecting the core part of CKAN Netkan Issues affecting the netkan data Network Issues affecting internet connections of CKAN Pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants