Thanks for the patch! This looks like a good idea to me. A few comments below.
Next time please send the patch to <~emersion/public-inbox@lists.sr.ht>, that
way other people can see it too. I realize this was missing from the README,
I've fixed that since then.
On Wednesday, February 15th, 2023 at 11:57, ~jamietanna <jamietanna@git.sr.ht> wrote:
> From: Jamie Tanna <jamie.tanna@deliveroo.co.uk>
>
> When using a schema that has `enum` strings, it can be handy to have all
> the enumeration's options available as constants.
>
> We can add support for generating a `const` block for the types that
> contain an `enum`.
>
> This currently only supports a top-level definition, but can be extended
> in the future for other types.
How could we adapt this for other types? The enumerated values in the JSON
schema are not named sadly. So for integers we may get e.g.
"$defs": {
"foo": {
"type": "integer",
"enum": [1, 42]
}
}
With no other hint regarding the meaning of the values.
I suppose maybe one could represent this as the following to give explicit
names to the values:
"$defs": {
"foo": {
"type": "integer",
"oneOf": [
{ "$ref": "#/$defs/fooBar" },
{ "$ref": "#/$defs/fooBaz" }
]
},
"fooBar": {
"const": 1
},
"fooBaz": {
"const": 42
},
}
That way we could generate Go constants with a meaningful name for "fooBar" and
"fooBaz".
Thoughts?
> ---
> cmd/jsonschemagen/main.go | 29 +++++++++++++++++++++++++++++
> go.mod | 5 ++++-
> go.sum | 1 +
> 3 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/jsonschemagen/main.go b/cmd/jsonschemagen/main.go
> index dde8ebe..c874f3b 100644
> --- a/cmd/jsonschemagen/main.go
> +++ b/cmd/jsonschemagen/main.go
> @@ -12,10 +12,14 @@ import (
> "unicode"
>
> "github.com/dave/jennifer/jen"
> + "golang.org/x/text/cases"
> + "golang.org/x/text/language"
I'd prefer to use strings.Title instead of adding a new dependency with
complicated conversion rules and support for many languages. We already
use that function elsewhere in this file. It's deprecated, if we want to get
rid of it we can easily copy it over, it's just a few lines of code.
> "git.sr.ht/~emersion/go-jsonschema"
> )
>
> +var titleCaser = cases.Title(language.English)
> +
> func formatId(s string) string {
> fields := strings.FieldsFunc(s, func(c rune) bool {
> return !unicode.IsLetter(c) && !unicode.IsNumber(c)
> @@ -229,7 +233,32 @@ func generateDef(schema *jsonschema.Schema, root *jsonschema.Schema, f *jen.File
> }
> } else {
> f.Type().Id(id).Add(generateSchemaType(schema, root, true)).Line()
> + enumType := generateEnumType(schema, id)
> + if enumType != nil {
> + f.Add(enumType)
> + }
> + }
> +}
> +
> +func generateEnumType(schema *jsonschema.Schema, name string) jen.Code {
> + if schema.Enum == nil || schemaType(schema) != jsonschema.TypeString {
Nit: I'd prefer a len(schema.Enum) check instead of comparing with nil, this
is less likely to result in confusing bugs when
schema.Enum = make([]interface{}, 0)
for instance.
> + return nil
> + }
> +
> + defs := make([]jen.Code, len(schema.Enum))
Hm, wouldn't this result in defs having a lot of nil entries, followed by the
entries added in the loop?
> + for _, v := range schema.Enum {
> + if s, ok := v.(string); ok {
> + constId := formatId(name + titleCaser.String(s))
> + def := jen.Id(constId).Op("=").Lit(s)
I think we need to specify the type here before the equal sign, otherwise the
entries will have the type `string`.
> + defs = append(defs, def)
> + }
> + }
> +
> + if defs == nil {
This is never hit, since we explicitly allocate the slice above. I think we
need a len() check instead.
> + return nil
> }
> + return jen.Const().Defs(defs...)
> }
Hey, sorry for the delay on getting back to this!
Thanks for that note re patches, this is the first contribution I've done on sr.ht, so wasn't aware of that, appreciate it!
> How could we adapt this for other types?
In oapi-codgen we've got the following generation semantics, for instance:
```yaml
name:
type: integer
description: Name of the pet
enum:
- 1
- 2
- 3
```
Gets generated as:
```go
// Defines values for PetName.
const (
PetNameN1 PetName = 1
PetNameN2 PetName = 2
PetNameN3 PetName = 3
)
```
Thoughts? Happy to point you to the code, but as it's Apache-2.0 not MIT like this repo, would be good to avoid you having any risk of licensing implications!
> I'd prefer to use strings.Title
As strings.Title is deprecated (https://stackoverflow.com/questions/71620717/in-go-1-18-strings-title-is-deprecated-what-to-use-now-and-how) so we need to use this as the recommended alternative.
As an aside, another thing that may be worth adding is installation instructions i.e.
```sh
go install git.sr.ht/~emersion/go-jsonschema/cmd/jsonschemagen@latest
```
On Sunday, February 19th, 2023 at 21:46, Jamie Tanna <jamie@jamietanna.co.uk> wrote:
> Thanks for that note re patches, this is the first contribution I've done on sr.ht, so wasn't aware of that, appreciate it!
No worries!
> > How could we adapt this for other types?
>
> In oapi-codgen we've got the following generation semantics, for instance:
>
> ```yaml
> name:
> type: integer
> description: Name of the pet
> enum:
> - 1
> - 2
> - 3
> ```
>
> Gets generated as:
>
> ```go
> // Defines values for PetName.
> const (
> PetNameN1 PetName = 1
> PetNameN2 PetName = 2
> PetNameN3 PetName = 3
> )
> ```
>
> Thoughts? Happy to point you to the code, but as it's Apache-2.0 not MIT like this repo, would be good to avoid you having any risk of licensing implications!
I am not a fan of this, because the N1/N2/N3 names don't actually convey more
meaning than literal 1/2/3 integers.
> > I'd prefer to use strings.Title
>
> As strings.Title is deprecated (https://stackoverflow.com/questions/71620717/in-go-1-18-strings-title-is-deprecated-what-to-use-now-and-how) so we need to use this as the recommended alternative.
Right, but the recommended alternative is overkill here.