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

Extract annotations into own package #1641

Merged
merged 8 commits into from
Oct 31, 2023

Conversation

trejjam
Copy link
Contributor

@trejjam trejjam commented Oct 30, 2023

Resolves #1628

@trejjam
Copy link
Contributor Author

trejjam commented Oct 30, 2023

In case you do not like Resolve NUKE build deprecation to be part of this PR I can move it to a separate PR

namespace NJsonSchema.Annotations;

/// <summary>Class containing the constants available as format string. </summary>
public static class JsonFormatStrings
Copy link
Owner

@RicoSuter RicoSuter Oct 31, 2023

Choose a reason for hiding this comment

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

I wonder whether we should put this in the NJsonSchema namespace but the NJsonSchema.Annotations pkg
and remove from NJsonSchema (and the same with JsonObjectType). What do you think? Might be confusing to have these two types twice... also I think it's not very nice/clean to have it twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand the motive, but it feels strange to exclude one attribute from the annotations package.

The duplicity is there because of the Newtonsoft attribute. I can solve it instead with JsonConverter for that type. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry no my propsal:

JsonFormatStrings: Keep in NJsonSchema.Annotations but with namespace NJsonSchema (remove in NJsonSchema pkg)
JsonObjectType: Keep in NJsonSchema.Annotations but with namespace NJsonSchema (remove in NJsonSchema pkg)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's ok for me. How do you prefer to handle these attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are from Newtonsoft package

Copy link
Owner

Choose a reason for hiding this comment

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

These? Link seems to be broken...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These: JsonPropertyAttribute

https://github.com/RicoSuter/NJsonSchema/blob/8b30fcd633389a56c10ac23c9287348775f849a8/src/NJsonSchema/JsonObjectType.cs#L26C1-L28C19

using Newtonsoft.Json;

namespace NJsonSchema
{
    /// <summary>
    /// Enumeration of the possible object types.
    ///
    /// Keep in sync with <see cref="Annotations.JsonObjectType"/>
    /// </summary>
    [Flags]
    public enum JsonObjectType
    {
        /// <summary>No object type. </summary>
        [JsonProperty("none")] // <----------------------------- this ---------------<
        None = 0,

        /// <summary>An array. </summary>
        [JsonProperty("array")] // <----------------------------- and this ---------------<
        Array = 1,
...
    }
}

Copy link
Owner

Choose a reason for hiding this comment

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

seems that these are not needed because of the magic here:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took me some time to find it :)

<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
Copy link
Owner

Choose a reason for hiding this comment

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

we should probably go with

netstandard2.0;net462

@@ -15,10 +15,10 @@ public class AnnotationClass
{
public MyPoint Point { get; set; }

[JsonSchema(JsonObjectType.String, Format = "point")]
[JsonSchema(Annotations.JsonObjectType.String, Format = "point")]
Copy link
Owner

Choose a reason for hiding this comment

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

can you use "using NJsonSchema.Annotations;" here?

@RicoSuter
Copy link
Owner

Ah ill update when it's merged.

@RicoSuter RicoSuter merged commit 8189415 into RicoSuter:master Oct 31, 2023
1 check passed
RicoSuter pushed a commit that referenced this pull request Oct 31, 2023
@RicoSuter
Copy link
Owner

Sorry for merging, want to publish a new preview package ASAP.

@RicoSuter
Copy link
Owner

v11.0.0-preview006

@trejjam
Copy link
Contributor Author

trejjam commented Oct 31, 2023

Sorry, I did not noticed unchecked checkbox:
image

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.

Create NJsonSchema.Attributes package
2 participants