kristiandupont / kanel

Generate Typescript types from Postgres
https://kristiandupont.github.io/kanel/
MIT License
881 stars 62 forks source link

Display pg type? #127

Closed rattrayalex closed 2 years ago

rattrayalex commented 3 years ago

I love that you print default value and index info in a docstring.

I noticed @slonik/typegen prints type information too.

This would probably be noise in many cases, like text for string, when there is one "obvious" mapping of the TS type to the PG type. But in on other cases, it might be nice to know, and have the info at your fingertips in your IDE as you develop. Eg;

interface Foo {
  /** 
   * Type: int 
   * Index: foo_bar
   */
  bar: number; 
}

What do you think?

rattrayalex commented 3 years ago

I also wonder if it might be nicer to use a tag style for these comments...

interface Foo {
  /** 
   * This is a comment from COMMENT ON.
   * @tag fixed
   * @pgtype int4
   * @index foo_bar
   * @default now()::int
   */
  bar: number; 
}

as opposed to:

interface Foo {
  /** 
   * This is a comment from COMMENT ON.
   * Tag: @fixed
   * Type: int4
   * Index: foo_bar
   * Default: now()::int
   */
  bar: number; 
}
kristiandupont commented 3 years ago

I assume you mean the pg type (int4, etc.)? I can see the value in that. Regarding tag-style comments, what would that gain? Does that show up in some special way somewhere?

rattrayalex commented 3 years ago

Sorry, yes – fixed!

Yeah, the tag style just looks different in many syntax highlighters, including GitHub's and VSCode's. It helps distinguish "this is an attribute of the thing the comment describes" as opposed to "This is some human-written prose talking about the thing the comment describes". It could also be used for certain kinds of automated tooling like a jsdoc-to-documentation-website generator, but that isn't my intent.

I don't feel strongly about the tag format, just wanted to suggest it since if you add every piece of information postgres knows about a table/column/etc, it might get unreadable without good delineation of some kind.

rattrayalex commented 3 years ago

Thinking about it a little more, since postgres doesn't seem to have any documented special handling of "smart comments" (like comment on column foo.bar is E'This is a docstring.\n@fixed', where @fixed is meant to be metadata), it wouldn't be appropriate to parse the comment and remove the @ symbol, and it would be confusing to have just @fixed appear on its own line in the comment while everything below it is not a part of the comment.

So, I agree this format might be better:

interface Foo {
  /** 
   * This is a docstring.
   * @fixed
   * Type: int4
   * Index: foo_bar
   * Default: now()::int
   */
  bar: number; 
}
kristiandupont commented 3 years ago

Yeah, the @-syntax is my own hacked solution. And I use it quite a bit for things that aren't exposed in Kanel at the moment. I am not sure I would want all of the tags listed in the comments (for instance, I could imagine a future where a given tag is translated to a typescript decorator and that would provide sufficient documentation in and of itself.

I guess the general purpose solution would be to expose another configurable function that generates the comments?

rattrayalex commented 3 years ago

Heh, the @-syntax isn't just yours! Also used in this project: https://www.graphile.org/postgraphile/smart-comments/

But, to be honest, I thought it was more widespread (eg; I misremembered that this was used in pg_hint_plan, which it's not).

I think my take is that pg comments should be printed as-is no matter what, without special parsing or exclusions (ie; @fixed should appear in the output). But, I also don't think the possible existence of a few @fixed comments should dissuade the use of @tag syntax in the JSDoc comments. After all, it's more likely that they appear on the first line of the comment anyway.

So, personally, I'd still recommend @default now()::int4 syntax over Default: now()::int4, but I also don't think it's worth adding configuration for, and I'd still be very happy with the project with the latter syntax.

Just my 2c 😄 thanks for patiently engaging on this!

kristiandupont commented 3 years ago

Hah, funny that they ended up with almost the exact same syntax! I guess I should align so that it has a better chance of becoming a standard of sorts. Thank you for pointing it out to me!

I see the argument in favor of leaving in the tags. I might change it or make it configurable.

kristiandupont commented 2 years ago

This should finally be addressed :-)

There is a premajor version out that allows you to specify what you want comments on both interfaces and properties to look like. There are significant breaking changes though, and the API is not final so feel free to experiment if you like. The documentation is work in progress still, you can start here: https://github.com/kristiandupont/kanel/blob/v3/docs/configuring.md

rattrayalex commented 2 years ago

Cool, that's awesome to hear! I'm not working with a db these days so I won't be able to test it myself sadly, but excited for the next pg project I have so I can try it out!