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

Get Supersets does not use color id, just gets all supersets #3

Open
Yoonwoo opened this issue Sep 19, 2021 · 4 comments
Open

Get Supersets does not use color id, just gets all supersets #3

Yoonwoo opened this issue Sep 19, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@Yoonwoo
Copy link

Yoonwoo commented Sep 19, 2021

The subsets function implements it with ?color_id parameter but supersets has color id just after / which is disregarded.

Can change to be like subsets function and fix:

public async Task<Superset[]> GetSupersetsAsync(ItemType type, string no, int colorId = 0)
        {
            var typeString = type.GetStringValueOrDefault();
            var builder = new UriBuilder(new Uri(_baseUri, $"items/{typeString}/{no}/supersets"));
            var query = HttpUtility.ParseQueryString(builder.Query);
            query["color_id"] = colorId.ToString();
            builder.Query = query.ToString();
            var url = builder.ToString();

            var method = HttpMethod.Get;
            var responseBody = await ExecuteRequest(url, method);

            var data = ParseResponse<Superset[]>(responseBody, 200, url, method);
            return data;
        }

In GetSupersetsDemo, example

var supersets = await client.GetSupersetsAsync(ItemType.Part, "40232", 8);

gets all supersets, not just color 8, works after changing

@gebirgslok
Copy link
Owner

Hi,

thanks for this catch. It's fixed and there will be a new NuGet package version soon.

Thanks very much

@Yoonwoo
Copy link
Author

Yoonwoo commented Sep 19, 2021

Cool, great client btw. Its a much higher standard than I could ever make.

@Yoonwoo
Copy link
Author

Yoonwoo commented Sep 19, 2021

Also not sure what you have in mind, but obviously its easy to make the function allow no color id and get all, it defaults to 0 for subsets and supersets which makes less sense for parts than it does for minifigures or sets.

if (colorId != -1)
            {
                var query = HttpUtility.ParseQueryString(builder.Query);
                query["color_id"] = colorId.ToString();
                builder.Query = query.ToString();
            }

@gebirgslok
Copy link
Owner

gebirgslok commented Sep 20, 2021

Cool, great client btw. Its a much higher standard than I could ever make.

Thanks. Just try to improve every day and never stop learning. This lib has a lot of things to improve as well (like injecting HttpClient, ServiceCollection extension etc.)

I'll look into that colorId = 0 issue again. The client should not be more complex than it has to (the server handles colorId = 0 anyway).

@gebirgslok gebirgslok reopened this Sep 20, 2021
@gebirgslok gebirgslok added the enhancement New feature or request label Sep 20, 2021
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

2 participants