chrusty / protoc-gen-jsonschema

Protobuf to JSON-Schema compiler
Apache License 2.0
496 stars 101 forks source link

ENUM source descriptions #98

Closed chrusty closed 3 years ago

chrusty commented 3 years ago

This PR introduces JSONSchema descriptions for ENUM fields, derived from relevant comments in their proto source code.

Thanks @maxlandon for raising this in https://github.com/chrusty/protoc-gen-jsonschema/issues/97

maxlandon commented 3 years ago

Thanks a lot !

maxlandon commented 3 years ago

yes give me a sec

maxlandon commented 3 years ago

it seems not to work, altough I might have made a mistake. Here are my steps: First out of my module (using your lib) i do go install protoc-gen-jsonschema@enum_source_descriptions I regenerate tags, but no descriptions in the schema Then, in my module, I do go get json...@enum_source, same thing: tags not generated. I checked the commit for both, its the right hash, so no problems with git Does it work on your side ?

maxlandon commented 3 years ago

Okay I know why: I thinking we misunderstood each other. The description for the enum name was already working well. What was (is) not produced is the descriptions for each enum value ! So I maybe wasn't clear on that.

I'm saying that based on my reading of your commit adding to tests: The descriptions for each enum should be along their respective values in the "enum": {"TCP": 0, "HTTP": 1} probably.

chrusty commented 3 years ago

OK cool, I will need to dig a bit deeper into this.

But actually the enum name bit wasn't working consistently, so that wasn't a waste of effort!

On Sat, 6 Nov 2021, 09:56 maxlandon, @.***> wrote:

Okay I know why: I thinking we misunderstood each other. The description for the enum name was already working well. What was (is) not produced is the descriptions for each enum value ! So I maybe wasn't clear on that.

I'm saying that based on my reading of your commit adding to tests: The descriptions for each enum should be along their respective values in the "enum": {"TCP": 0, "HTTP": 1} probably.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/chrusty/protoc-gen-jsonschema/pull/98#issuecomment-962214826, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOQMNGF36W54NNZUDQTBTDUKRHIPANCNFSM5HOTOBQQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

maxlandon commented 3 years ago

Okay thanks a lot, I'm here when you need for testing commits !

chrusty commented 3 years ago

Hi @maxlandon, just had another look at this.

I'm not sure that it will be possible to include a description for each ENUM value because a JSONSchema ENUM is just an array of values (not an array of structures which I could attach individual descriptions to): https://json-schema.org/understanding-json-schema/reference/generic.html#enumerated-values

The only thing I can think of would be to encode this into a big text description field for the ENUM as a whole which could be messy and quite large in certain circumstances.

What do you think?

maxlandon commented 3 years ago

let me check that. You're right there's a problem

maxlandon commented 3 years ago

Here is a trail I'm following: https://github.com/json-schema-org/json-schema-spec/issues/57

maxlandon commented 3 years ago

I'm trying their

│       "Type": {
│       │   "oneOf": [
│       │       │   { "const": "Session", "description": "This is a Session description" },
│       │       │   { "const": 0, "description": "This is a Session description" },
│       │       │   { "const": "Beacon", "description": "This is a Beacon description" },
│       │       │   { "const": 1, "description": "This is a Beacon description" }
│       │   ],

I'm just saving the Schema as is, but it doesn't work in my editor. So I don't really know how to test that... I'm not quite familiar with how subschemas work, so part of the thread I linked to is a bit cryptic to me !

chrusty commented 3 years ago

I'm trying their

│       "Type": {
│       │   "oneOf": [
│       │       │   { "const": "Session", "description": "This is a Session description" },
│       │       │   { "const": 0, "description": "This is a Session description" },
│       │       │   { "const": "Beacon", "description": "This is a Beacon description" },
│       │       │   { "const": 1, "description": "This is a Beacon description" }
│       │   ],

I'm just saving the Schema as is, but it doesn't work in my editor. So I don't really know how to test that... I'm not quite familiar with how subschemas work, so part of the thread I linked to is a bit cryptic to me !

After skimming through that trail for the json-schema-spec I think I agree - the const approach seems to be the cleanest option, but for compatibility I'd still have to provide the "ENUM" array too. In any case this probably isn't going to happen any time soon because const doesn't seem to be supported by the jsonschema library I'm using in this plugin.

Interestingly there was an issue raised there a while back along these same lines.