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

Serialize time decimals using invariant culture (always use period as decimal separator) #35

Merged
merged 1 commit into from
May 25, 2021

Conversation

thj-dk
Copy link
Contributor

@thj-dk thj-dk commented Apr 13, 2021

Fixes #34 time bug where current culture is taken into account when serializing time decimals. Jenkins expects time decimals to use a period as decimal separator.

@Siphonophora
Copy link
Collaborator

@thj-dk Thanks, this is pretty much what I expected. Were you able to do any direct testing with this branch to confirm this fixes the issue?

@thj-dk
Copy link
Contributor Author

thj-dk commented Apr 14, 2021

@Siphonophora Yes. I tested the produced package from my branch using a local nuget repository. Using both cultures da-DK and nb-NO.
You can see the different results here: https://gist.github.com/thj-dk/4596679994b0176b6c1ba4ef4534f388

@thj-dk
Copy link
Contributor Author

thj-dk commented Apr 16, 2021

@Siphonophora any chance of getting this fix merged and released? It's a bit of a deal breaker for us if we should use this lib or not.

@Siphonophora
Copy link
Collaborator

@thj-dk Yeah, I've been thinking about whether there is an easy way to produce a test that would fail without this. I think that the JunitXmlValidator would probably raise an error if it was handed the version with commas instead of decimals.

That test would probably need to just call the serializer, directly, so we could control the thread culture, as it doesn't look easy to do that with the full acceptance test where we spin up an external process. If you want to give this a look, let me know. Otherwise I will.

I should have time to add the CLI flag over the weekend. In terms of release, what we would do is produce a pre-release package as these changes get merged down and ask you to verify the solution in your environment before moving to nuget.

@thj-dk
Copy link
Contributor Author

thj-dk commented Apr 19, 2021

@Siphonophora I can give the test a shot, but I cannot promise when. I'm a bit busy at the moment unfortunately.

@Siphonophora
Copy link
Collaborator

Siphonophora commented Apr 19, 2021

@thj-dk Understood if you don't have much time. If you can only spend a little time I would rather ask you to spend it on #38 (which isn't time sensitive) and I can do this soon.

@thj-dk
Copy link
Contributor Author

thj-dk commented May 18, 2021

@Siphonophora any chance on getting a new version released with this PR?

@Siphonophora
Copy link
Collaborator

@thj-dk Yes, I will get that sorted this weekend. I have been thinking that because #39 doesn't look like a comprehensive solution to #34 that I will go ahead and remove that for now. This issue is a clear permanent fix and I'm happy to remove it. Let me know what you think, but I expect it will still be a while until #34 is fully sorted. If you have more feedback on that, it would be helpful.

@Siphonophora Siphonophora merged commit 2b21a9f into spekt:master May 25, 2021
@Siphonophora
Copy link
Collaborator

@codito Can you cut a release? The only real change here is the bugfix from @thj-dk

@thj-dk
Copy link
Contributor Author

thj-dk commented May 26, 2021

@thj-dk Yes, I will get that sorted this weekend. I have been thinking that because #39 doesn't look like a comprehensive solution to #34 that I will go ahead and remove that for now. This issue is a clear permanent fix and I'm happy to remove it. Let me know what you think, but I expect it will still be a while until #34 is fully sorted. If you have more feedback on that, it would be helpful.

Thanks man. Regarding #34, i'm very busy at the moment, so I haven't got much time. We've currently sorted the problem using a custom powershell script that modifies the outputted xml to our needs. That'll do for now.

@thj-dk thj-dk deleted the bugfix/time-decimal-culture branch May 26, 2021 06:00
@codito
Copy link
Contributor

codito commented May 27, 2021

@codito Can you cut a release? The only real change here is the bugfix from @thj-dk

Done, v3.0.98 should be on nuget in a few minutes.

@thj-dk
Copy link
Contributor Author

thj-dk commented May 27, 2021

Thanks guys. Verified with success!

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.

Indicate framework in produced xml file
3 participants