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

Yaml Comment. #584

Merged
merged 3 commits into from
Mar 30, 2021
Merged

Yaml Comment. #584

merged 3 commits into from
Mar 30, 2021

Conversation

PANDATEEN
Copy link
Contributor

add a description attribute about property.
if used,when Serialize(),the result will have a yaml comment on that property.

add a description attribute about property.
if used,when Serialize(),the result will have a yaml comment on that property.
add a description attribute about property.
if used,when Serialize(),the result will have a yaml comment on that property.
@aaubry
Copy link
Owner

aaubry commented Feb 22, 2021

Thanks for this. I'll try to fix the CI so that the build passes. Could you add some tests to show that this works as intended ?

add a xunit test .cs file about yaml comments.
it`s just an easy comment above the property when Serialize(*).
@PANDATEEN
Copy link
Contributor Author

add a test about that.
please check if convenient.

@pawelpawlow
Copy link

@aaubry, is there any chance we have this merged and released soon?

@aaubry
Copy link
Owner

aaubry commented Mar 30, 2021 via email

@aaubry
Copy link
Owner

aaubry commented Mar 30, 2021

I'm going to merge this, but I made some changes that you can check here:

  1. I moved the functionality to a new class, as this has noting to do with handling default values, which is the responsibility of DefaultValuesObjectGraphVisitor.
  2. I updated the test to actually perform assertions. Otherwise the test was not testing anything.

@PANDATEEN
Copy link
Contributor Author

What dou you mean by "Otherwise the test was not testing anything"?
It at least test the great courage of mine.

@aaubry
Copy link
Owner

aaubry commented Mar 31, 2021

I'm sorry, I didn't mean to sound dismissive. What I mean is that the test was not making any assertion. This means that if a later change breaks the functionality, we won't find out unless we remember to check the output of the test.
I added assertions that check that the YAML at least contains the comments. This way the build will fail if someone breaks this feature.

@aaubry
Copy link
Owner

aaubry commented Mar 31, 2021

This feature has just been released, in YamlDotNet 10.1.0.

@ahannani-rms
Copy link

ahannani-rms commented Mar 31, 2021

@aaubry - I just have a follow up question about this merge: is this change what may have caused a broken reference to be added to Yamldotnet's index.json in https://www.nuget.org?

We saw reference resolution issues during dotnet restore that stemmed from HTTP 404 on the following URI:
https://api.nuget.org/v3/registration5-gz-semver2/yamldotnet/page/8.0.0/10.0.1-pandateen-master-0006.json

@aaubry
Copy link
Owner

aaubry commented Mar 31, 2021

I'm not sure what you mean. What is index.json ?

@ahannani-rms
Copy link

The nuget manifest for your package in the public nuget repository: https://api.nuget.org/v3/registration5-gz-semver2/yamldotnet/index.json.

It now looks correct, but yesterday when we started seeing build failures shortly after this merge, it looked like the following:

{
	"@id": "https://api.nuget.org/v3/registration5-gz-semver2/yamldotnet/index.json",
	"@type": ["catalog:CatalogRoot", "PackageRegistration", "catalog:Permalink"],
	"commitId": "3ad1220b-820c-4fc6-a971-c28c12e43d70",
	"commitTimeStamp": "2021-03-30T12:32:22.6862928+00:00",
	"count": 4,
	"items": [{
		"@id": "https://api.nuget.org/v3/registration5-gz-semver2/yamldotnet/page/2.3.0-rc/3.9.0-pre239.json",
		"@type": "catalog:CatalogPage",
		"commitId": "a86247e0-dc5c-47a4-9c34-517c8086789a",
		"commitTimeStamp": "2021-02-02T12:14:33.9174985+00:00",
		"count": 64,
		"lower": "2.3.0-rc",
		"upper": "3.9.0-pre239"
	}, {
		"@id": "https://api.nuget.org/v3/registration5-gz-semver2/yamldotnet/page/3.9.0-pre240/4.1.1-pre0355.json",
		"@type": "catalog:CatalogPage",
		"commitId": "a86247e0-dc5c-47a4-9c34-517c8086789a",
		"commitTimeStamp": "2021-02-02T12:14:33.9174985+00:00",
		"count": 64,
		"lower": "3.9.0-pre240",
		"upper": "4.1.1-pre0355"
	}, {
		"@id": "https://api.nuget.org/v3/registration5-gz-semver2/yamldotnet/page/4.1.1-pre0356/8.0.0-pre0572.json",
		"@type": "catalog:CatalogPage",
		"commitId": "a86247e0-dc5c-47a4-9c34-517c8086789a",
		"commitTimeStamp": "2021-02-02T12:14:33.9174985+00:00",
		"count": 64,
		"lower": "4.1.1-pre0356",
		"upper": "8.0.0-pre0572"
	}, {
		"@id": "https://api.nuget.org/v3/registration5-gz-semver2/yamldotnet/page/8.0.0/10.0.1-pandateen-master-0006.json",
		"@type": "catalog:CatalogPage",
		"commitId": "3ad1220b-820c-4fc6-a971-c28c12e43d70",
		"commitTimeStamp": "2021-03-30T12:32:22.6862928+00:00",
		"count": 16,
		"lower": "8.0.0",
		"upper": "10.0.1-PANDATEEN-master-0006"
	}],
	"@context": {
		"@vocab": "http://schema.nuget.org/schema#",
		"catalog": "http://schema.nuget.org/catalog#",
		"xsd": "http://www.w3.org/2001/XMLSchema#",
		"items": {
			"@id": "catalog:item",
			"@container": "@set"
		},
		"commitTimeStamp": {
			"@id": "catalog:commitTimeStamp",
			"@type": "xsd:dateTime"
		},
		"commitId": {
			"@id": "catalog:commitId"
		},
		"count": {
			"@id": "catalog:count"
		},
		"parent": {
			"@id": "catalog:parent",
			"@type": "@id"
		},
		"tags": {
			"@id": "tag",
			"@container": "@set"
		},
		"reasons": {
			"@container": "@set"
		},
		"packageTargetFrameworks": {
			"@id": "packageTargetFramework",
			"@container": "@set"
		},
		"dependencyGroups": {
			"@id": "dependencyGroup",
			"@container": "@set"
		},
		"dependencies": {
			"@id": "dependency",
			"@container": "@set"
		},
		"packageContent": {
			"@type": "@id"
		},
		"published": {
			"@type": "xsd:dateTime"
		},
		"registration": {
			"@type": "@id"
		}
	}
}

Please note the 10.0.1-pandateen-master-0006 item.

@aaubry
Copy link
Owner

aaubry commented Mar 31, 2021

I don't know. For feature branches, the CI is configured to publish a pre-release package so that users can test the feature. This is what happened here, but I wouldn't expect this to cause problems.
Note that there seems to be a bug in nuget. If you happen to download the metadata while it is being updated, you get incomplete data in your cache and then you have to manually clear the cache.

@ahannani-rms
Copy link

ahannani-rms commented Mar 31, 2021

Thanks @aaubry! Yeah, I actually ended up nuking the cache from our central artifactory as well as the local cache, to no avail. It kept getting refreshed with the same references from the public nuget repo, which leads me to believe the problem existed at the source (nuget.org).
However, this morning things got corrected.

@aaubry
Copy link
Owner

aaubry commented Mar 31, 2021

I'm glad that the problem went away. I will keep an eye on the feed to see if this happens again. Thanks

@ahannani-rms
Copy link

Much appreciated!

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.

4 participants