romgrk / node-gtk

GTK+ bindings for NodeJS (via GObject introspection)
MIT License
493 stars 41 forks source link

Multi-word constructor props should be camelCase, not snake_case #320

Open Lesik opened 2 years ago

Lesik commented 2 years ago

In order to pass the property "icon name" to the constructor of Gtk.Image, as an example, the property name must be icon_name:

new Gtk.Image ({
  "icon_name": "folder",
});

But it should be iconName according to the types generated by ts-for-gjs, and also as you likely know it's idiomatic for JS.

export class Image {
    /* Properties of Gtk.Image */
    // snip
    constructor (config?: Image_ConstructProps),
    // snip
}

export interface Image_ConstructProps extends Misc_ConstructProps {
    // snip
    iconName?: string
    // snip
}

(Really loving the passing of props through the config object btw, very idiomatic, thanks!)

Passing iconName results in a crash with the following message:

Error: Invalid property name: iconName

System information:

$ apt-cache show libgtk-3-0
Package: libgtk-3-0
Source: gtk+3.0
Version: 3.24.24-4
$ grep 'node-gtk' package.json
    "node-gtk": "^0.10.0",

Edit: To clarify, this has nothing to do with Gtk.Image, it holds true for all widgets that have multi-word constructor props.

binyamin commented 2 years ago

Have you seen the naming conventions? It seems this behavior is intended. As for ts-for-gjs, that's a separate project by a separate developer. If the typedefs don't match up to node-gtk, I would open an issue there.

Lesik commented 2 years ago

@binyamin Maybe I'm not reading it right, but it looks like the naming conventions only strengthen my point. There's isn't any entry in the naming conventions that would use lowercase snake_case, and according to it, properties should be camelCased.

binyamin commented 2 years ago

@Lesik hmm, here's the particular convention I was referring to (italics are my own). If I'm reading it right, maybe there should be a lowercase example.

  • Constants & Values: SNAKE_CASE (not modified, may contain lowercase) Can be attached on namespaces or on specific objects. Example: Gdk.KEY_g !== Gdk.KEY_G Gdk.EventType.KEY_PRESS
Lesik commented 2 years ago

@binyamin No offense, but I think it would be quite a stretch of definition to call properties constants. Example:

new Gtk.Image({
    "icon_size": Gtk.IconSize.LARGE_TOOLBAR,
});

In this example, Gtk.IconSize.LARGE_TOOLBAR is a constant/value, while icon_size is a property. And properties should be lowerCamelCase:

  • Fields & Properties: lowerCamelCase Fields are on structs and unions. Properties are on GObjects. Example: textView.showLineNumbers = true new Gdk.Color().blue = 200
binyamin commented 2 years ago

Great point. I'm going to hand this one off to @romgrk

romgrk commented 2 years ago

@Lesik is right, the behavior is buggy. If it's object.propName to access the property, it should be new Object({ propName }) to initialize it. We should change it, but we can keep the snake case working for backwards compatibility for existing code.