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

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

Open
Lesik opened this issue Dec 3, 2021 · 6 comments
Open

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

Lesik opened this issue Dec 3, 2021 · 6 comments

Comments

@Lesik
Copy link

Lesik commented Dec 3, 2021

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
Copy link
Collaborator

binyamin commented Dec 5, 2021

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
Copy link
Author

Lesik commented Dec 9, 2021

@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
Copy link
Collaborator

binyamin commented Dec 9, 2021

@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
Copy link
Author

Lesik commented Dec 9, 2021

@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
Copy link
Collaborator

binyamin commented Dec 9, 2021

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

@romgrk
Copy link
Owner

romgrk commented Jan 8, 2022

@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.

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

No branches or pull requests

3 participants