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

Culture specific bugs are fixed. All tests are "green" now. #7

Closed
wants to merge 0 commits into from

Conversation

psmolich79
Copy link
Contributor

Fixed problems related to culture and proper conversions.

@iwate
Copy link
Owner

iwate commented Jul 26, 2019

Hi @psmolich79 ! Thank you for your contribute!

I check your code and it looks good :)

However, I want to know what values cause error.

Would you add a few test cases specific values ?

@bxbxbx
Copy link

bxbxbx commented Aug 4, 2021

Hi @iwate please consider to merge this pull request, as it fixes formatting problems on non english systems.
The source of the problems is that the actual culture is used in the parameterizers, which leads to problems with floating point numbers on non english systems.

@@ -57,7 +57,8 @@ public static Request Create<T>(HttpMethod method, string uri, T body, IEnumerab

if (type.IsValueType || type == stringType)
{
content = body.ToString();
//content = body.ToString();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This need plain text, so we cannot use json serializer.

iwate added a commit that referenced this pull request Aug 5, 2021
@iwate
Copy link
Owner

iwate commented Aug 5, 2021

@bxbxbx Thank you for your comments.

I understand this current culture issue but I cannot merge this PR because this patch has a many changes from current master.

I've added red pattern test codes for this issue. (597337a) If @psmolich79 could modify and recreate this PR, I merge it.

When this PR is not update until next weekend, I create a patch by self and close this PR.

@psmolich79 psmolich79 requested a review from iwate August 6, 2021 13:41
Copy link
Owner

@iwate iwate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@psmolich79 Thank you modify PR.

However, this change includes a change that reverts to the old code.

Could you re-create it including only your changes?

If it is difficult, you can create new a branch from current master and create new a PR.

}
else if (type.IsValueType)
{
content = serializer.Serialize<T>(body);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This request content type is needed plain text whichi cannnot use serializer. use ToString with CultureInfo.InvariantCulture.

@@ -16,10 +14,8 @@ public class Response
public string MediaType { get; private set; }
public string ErrorMessage { get; private set; }
public byte[] Binary { get; private set; }
public string Body { get => GetStringFromUTF8(Binary); }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetStringFromUTF8 is needed for BOM

@iwate
Copy link
Owner

iwate commented Aug 7, 2021

This issue is resolved at #24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants