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

feat: add cultureinfo to reader/writer settings. #152

Merged
merged 15 commits into from
Mar 30, 2024

Conversation

ByronMayne
Copy link
Contributor

About the PR

The unit tests are currently failing to run on linux but work on windows due to differences with date time parsing and newlines. In this PR I have done the following

  • Created a static Configuration class that contains common settings used across all the project
    • Contains CultureInfo instead of being hard coded across the code base
    • Contains DateTimeFormat which is explicitly ISO 8601 and used for reading and writing
  • Removed StringExtensions.cs and instead opted for extending Fluent Assertions FluentAssersionExtensions
    • Across all unit tests the same functions were used for fixing new line characters. Instead of doing this I extend fluent assertions which cleans up the code.
  • General Test Cleanup
    • Moved some of the inline test data into stand alone files. Having massive text embedded in code makes they really hard to read and follow up on.

Changelog

  • Add: DataTime is always read and written using ISO 8601 format

Follow up questions

  • The unit tests currently all tests everything in one go rather then individual components. When they break it is very tedious to figure out which part went wrong. Are you against breaking them apart?
  • Across the application there is a few one off static helpers JsonHelper, YamlConverter, and previously StringExtensions. Would it make sense to combine them into either the Configuration in this review or have them as an instance that is passed around as a context?

@ByronMayne
Copy link
Contributor Author

As per usual feel free to push back on anything you don't agree with :)

@ByronMayne ByronMayne changed the title fix: Fixed unit tests across across all platforms fix: fixed unit tests across across all platforms Mar 25, 2024
@VisualBean VisualBean changed the title fix: fixed unit tests across across all platforms test: fixed unit tests across across all platforms Mar 25, 2024
Copy link
Collaborator

@VisualBean VisualBean left a comment

Choose a reason for hiding this comment

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

Only a few things, but otherwise I am happy with it

@VisualBean
Copy link
Collaborator

Definitely not against splitting unit tests apart.
I think we should add a document test for the kafka streetlights example as a default document test, and migrate everything else out into smaller units, for sure.

@VisualBean VisualBean changed the title test: fixed unit tests across across all platforms feat: add cultureinfo to reader/writer settings. Mar 30, 2024
@VisualBean
Copy link
Collaborator

Thanks a ton @ByronMayne, great addition to the project.

@VisualBean VisualBean merged commit 0199420 into LEGO:main Mar 30, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants