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

Wrapper Updates #4

Merged
merged 8 commits into from Nov 19, 2019
Merged

Wrapper Updates #4

merged 8 commits into from Nov 19, 2019

Conversation

@exsersewo
Copy link
Member

exsersewo commented Nov 10, 2019

Summary:
Add More Client Tests,
Added Interface,
Add Comments,
Redid PostImage Response

EDIT:
Also contains experimental multipart uploading on files >= 1mb

@exsersewo exsersewo requested a review from velddev Nov 10, 2019
exsersewo added 3 commits Nov 11, 2019
Add Experimental Flag,
Remove Miki.Http usings for now
Add Multipart Form Data
Update README.md to reflect changes to project
@@ -10,9 +10,29 @@ public class ClientTests
[Fact]
public void ConstructClientTest()
{
var i = new ImghoardClient(ImghoardClient.Config.Default());
var i = new ImghoardClient(Config.Default());

This comment has been minimized.

Copy link
@velddev

velddev Nov 12, 2019

Member

Could we make it so that if you pass null it will fallback to null? this way we could make it optional to pass a Config.

This comment has been minimized.

Copy link
@exsersewo

exsersewo Nov 12, 2019

Author Member

By default, if the constructor is empty, it uses the default configuration, this was just a straggler that was oversighted from testing.

{
public string Tenancy { get; set; } = "production";
public string Endpoint { get; set; } = "https://api.miki.ai/";
public string UserAgent { get; set; } = $"Mozilla/5.0 (Imghoard.Net/{Assembly.GetExecutingAssembly().GetName().Version.ToString().Substring(0, 3)}; +https://github.com/Mikibot/dotnet-imghoard)";

This comment has been minimized.

Copy link
@velddev

velddev Nov 12, 2019

Member

Why Mozilla/5.0? This is not a web browser.

/// <returns>The url of the uploaded image or null on failure</returns>
public async Task<string> PostImageAsync(Memory<byte> bytes, params string[] Tags)
{
if(bytes.Length >= 1000000 && !config.Experimental)

This comment has been minimized.

Copy link
@velddev

velddev Nov 12, 2019

Member

Preferably do not use 'magic numbers'. Recommend 10 * Mb where Mb is a constant.

}

var b64 = Convert.ToBase64String(bytes);
image.Position = 0;
(bool supported, string prefix) = IsSupported(bytes.Span.ToArray());

This comment has been minimized.

Copy link
@velddev

velddev Nov 12, 2019

Member

This copies the array twice. I would recommend for the future to move IsSupported to a span/memory as well.

Data = $"data:image/{prefix};base64,{Convert.ToBase64String(bytes.Span)}",
Tags = Tags
},
new JsonSerializerSettings

This comment has been minimized.

Copy link
@velddev

velddev Nov 12, 2019

Member

Consider making this a constant

var response = await apiClient.PostAsync("/images", body);

if (response.Success)
return null;

This comment has been minimized.

Copy link
@velddev

velddev Nov 12, 2019

Member

Do we want to return null? or throw an exception?

{
public class ImagesResponse
{
private readonly ImghoardClient ClientInstance;

This comment has been minimized.

Copy link
@velddev

velddev Nov 12, 2019

Member

private variables should be lowerCamelCase

Copy link
Member Author

exsersewo left a comment

I believe all of these have been resolved

@exsersewo exsersewo requested a review from velddev Nov 12, 2019
Copy link
Member

velddev left a comment

Good progress!

@@ -10,9 +10,29 @@ public class ClientTests
[Fact]
public void ConstructClientTest()

This comment has been minimized.

Copy link
@velddev

velddev Nov 13, 2019

Member

This test doesn't make much sense.

This comment has been minimized.

Copy link
@velddev

velddev Nov 13, 2019

Member

Would recommend to verify if there's something like the client set up, or the correct URL is being propagated

src/Imghoard.Tests/ClientTests.cs Outdated Show resolved Hide resolved
src/Imghoard/Config.cs Outdated Show resolved Hide resolved

apiClient = new HttpClient();
apiClient.DefaultRequestHeaders.Add("x-miki-tenancy", config.Tenancy);
apiClient.DefaultRequestHeaders.Add("User-Agent", config.UserAgent);

This comment has been minimized.

Copy link
@velddev

velddev Nov 13, 2019

Member

why is this capp'd, but the other one isn't?


if (page > 0)
{
query.Append($"page={page}");

This comment has been minimized.

Copy link
@velddev

velddev Nov 13, 2019

Member

Would recommend to rewrite this as

List<string> args = new List<string>();

if(page > 0) 
{
    args.Add($"page={page}");
}
if(tags.Any()) 
{
    args.Add($"tags={string.Join("+", tags)}")
}

string endpoint = baseEndpoint;
if(args.Any())
{
    endpoint += "?" + string.Join("&", args);
}
src/Imghoard/ImghoardClient.cs Outdated Show resolved Hide resolved
src/Imghoard/ImghoardClient.cs Outdated Show resolved Hide resolved
src/Imghoard/Exceptions/ResponseException.cs Outdated Show resolved Hide resolved
src/Imghoard/Config.cs Outdated Show resolved Hide resolved
@exsersewo exsersewo requested a review from velddev Nov 17, 2019
src/Imghoard/Models/ImagesResponse.cs Outdated Show resolved Hide resolved
src/Imghoard/ImghoardClient.cs Outdated Show resolved Hide resolved
src/Imghoard/ImghoardClient.cs Outdated Show resolved Hide resolved
{
var mock = new Mock<IImghoardClient>();

mock.Setup(x => x.GetImageAsync(It.IsAny<ulong>()))

This comment has been minimized.

Copy link
@velddev

velddev Nov 18, 2019

Member

Technically here you want to mock the http client and inject it into the imghoard client

@velddev

This comment has been minimized.

Copy link
Member

velddev commented Nov 18, 2019

Technically I'm not against pushing this, but I added these and want to know what you think @exsersewo

@exsersewo exsersewo requested a review from velddev Nov 19, 2019
@exsersewo exsersewo merged commit 1746e13 into master Nov 19, 2019
@exsersewo exsersewo deleted the pagination branch Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.