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 Json converter for serialzing models that contain empty strings and empty lists. #40

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

JonasSchneegans
Copy link

No description provided.

Copy link
Contributor

@hf-kklein hf-kklein left a comment

Choose a reason for hiding this comment

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

  • Ich würde mir noch dokumentation wünschen für die klassen. nur highlevel an der klasse selbst, wozu sie gut sind und was unsere annahmen sind
  • an einer stelle fehlt eine rekursion würde ich vermuten
  • die write-pfade können wir weglassen oder es sollte ein test fehlschlagen, wenn wir sie rausnehmen. da wir im test bislang nur deserializen, vemrute ich dass das grün durchliefe: update: nein, ist nicht so. wir brauchen sie. haben wir dann jeweils beide fälle anders wo abgedeckt in einem test? vllt ließe sich da noch ein testfall adden.

gerne kannst du auch die converter abseits der malo-ident model klassen testen. ich mach das gerne, dass ich mir einfache test-klassen konstruiere, wie z.b. hier: https://github.com/Hochfrequenz/BO4E-dotnet/blob/50ceed1ba0d2c86e63a74929aaf40e730db79558/BO4ETestProject/TestGeraetemerkmalConverter.cs#L20-L32

das ist teilweise weniger overhead als sich "echte" malo-ident testdaten zu bauen und die intention des testes wird klarer und der konverter punktgenau getestet.

@hf-kklein hf-kklein changed the title Add Json converter for serialzing models that contain empty strings and empty lists. feat: Add Json converter for serialzing models that contain empty strings and empty lists. Feb 25, 2025
@JonasSchneegans
Copy link
Author

@hf-kklein Morgen, passt das so jetzt für dich?

namespace MaLoIdentModels.JsonSettings;

/// <summary>
/// A custom JSON converter that converts empty strings to null and writes null values as empty strings.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// A custom JSON converter that converts empty strings to null and writes null values as empty strings.
/// A custom JSON converter that converts empty strings to null and writes empty strings as null values.
if (string.IsNullOrEmpty(value))
        {
            writer.WriteNullValue();
        }
        else
        {
            writer.WriteStringValue(value);
        }

siehe unten

Copy link
Contributor

@hf-kklein hf-kklein left a comment

Choose a reason for hiding this comment

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

ein kommentar ist noch falsch. danke für die zusätzlichen tests! gerne nach dem merge die minor version bummpen

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.

3 participants