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

Easy customization of generated code through YAML file #235

Open
filipowm opened this issue Jan 3, 2025 · 3 comments
Open

Easy customization of generated code through YAML file #235

filipowm opened this issue Jan 3, 2025 · 3 comments

Comments

@filipowm
Copy link
Contributor

filipowm commented Jan 3, 2025

It would be nice to have simplified way of customizing generated code and fields, I mean this part in particular (ref: https://github.com/paultyng/go-unifi/blob/main/fields/main.go#L342-L452)

switch resource.StructName {
		case "Account":
			resource.FieldProcessor = func(name string, f *FieldInfo) error {
				switch name {
				case "IP", "NetworkID":
					f.OmitEmpty = true
				}
				return nil
			}
		case "ChannelPlan":
			resource.FieldProcessor = func(name string, f *FieldInfo) error {
				switch name {
				case "Channel", "BackupChannel", "TxPower":
					if f.FieldType == "string" {
						f.CustomUnmarshalType = "numberOrString"
					}
				}
				return nil
			}
                (...)
}

so that it's easier to maintain and modify by external contributors. My idea is to be able to codify it in simple yaml file in following structure:

customizations:
  <resource_name>:
    fields:
      _all: # meta marker to apply these changes to all
      <field_name>:
        fieldName: "" # override field name
        fieldType: "" # override field type
        customUnmarshalType: "" # override unmarshal type
        customUnmarshalFunc: "" # override unmarshal func
        omitEmpty: true # override omitEmpty flag
        ifFieldType: "" # apply changes only if field is of specified type

This way we could have customizations.yml like:

customizations:
  Account:
    fields:
      IP:
        omitEmpty: true
      NetworkID:
        omitEmpty: true
  ChannelPlan:
    fields:
      Channel:
        ifFieldType: "string"
        customUnmarshalType: "numberOrString"
      BackupChannel:
        ifFieldType: "string"
        customUnmarshalType: "numberOrString"
      TxPower:
        ifFieldType: "string"
        customUnmarshalType: "numberOrString"
  Device:
    fields:
      _all:
        omitEmpty: true
      X:
        fieldType: "float64"
      Y:
        fieldType: "float64"
      StpPriority:
        fieldType: "string"
        customUnmarshalType: "numberOrString"
      Ht:
        fieldType: "int"
      Channel:
        customUnmarshalType: "numberOrString"
        ifFieldType: "string"
      BackupChannel:
        customUnmarshalType: "numberOrString"
        ifFieldType: "string"
      TxPower:
        customUnmarshalType: "numberOrString"
        ifFieldType: "string"
      LteExtAnt:
        customUnmarshalType: "booleanishString"
      LtePoe:
        customUnmarshalType: "booleanishString"
      PortOverrides:
        omitEmpty: false

Order of processing:

graph LR;
    all[_all customization]
    fields[field-specific customization]
    custom[coded customization]
    all-->fields-->custom
Loading

where each subsequent customization could amend previous ones.

What do you think @joshuaspence ? If you're fine with idea, I can jump on that and draft some code.

@joshuaspence
Copy link
Collaborator

I am not opposed to the idea but also not sold on it either. I've always treated this code as a means to an end in that I haven't tried to make it clean since at the end of the day the only thing that matters is the generated files it produces. So I'd probably be opposed to say doubling the amount of code without adding any functionality (just an example) but if the amount of code is comparable then why not.

Having said that, I see this as fairly low value. I haven't had a chance to look at it at all but I noticed this in the release notes for 9.0.108:

Network Application API
The API provides powerful tools to manage Sites, Devices, and Clients, offering access to detailed configuration, real-time status, and live statistics. It supports insights for WiFi, Wired, and VPN clients, including connection details

This could potentially obsolete this code, if not a large part of go-unifi.

@filipowm
Copy link
Contributor Author

filipowm commented Jan 3, 2025

I am not opposed to the idea but also not sold on it either. I've always treated this code as a means to an end in that I haven't tried to make it clean since at the end of the day the only thing that matters is the generated files it produces. So I'd probably be opposed to say doubling the amount of code without adding any functionality (just an example) but if the amount of code is comparable then why not.

Having said that, I see this as fairly low value. I haven't had a chance to look at it at all but I noticed this in the release notes for 9.0.108:

Network Application API
The API provides powerful tools to manage Sites, Devices, and Clients, offering access to detailed configuration, real-time status, and live statistics. It supports insights for WiFi, Wired, and VPN clients, including connection details

This could potentially obsolete this code, if not a large part of go-unifi.

Yup, I agree that it's not huge value as of now. I'd just keep this as an idea once we will be adding more features, yet as you mentioned 9.0.108 may be a major gamechanger (and it popped up as new years gift). We can come back to the idea when the amount of customizations will be growing 👍

Yet I didn't have chance to look into new version, I will try to update my FW in upcoming days and check that to see the impact on go-unifi. I hope they caught up with all reverse-engineering efforts to finally publish official API, however I can't find any documentation of it yet (or I'm not digging well enough 😅 )

@joshuaspence
Copy link
Collaborator

joshuaspence commented Jan 7, 2025

I will create a separate GitHub issue regarding the official API but coming back to your specific suggestions here, I'd actually prefer to move in the opposite direction where we move away from customisations or, if necessary, use generic transformations rather than per-resource customisations.

I worry that making it easier to customise the generated code like this takes us further away from code generation and at some point maybe we are better off just hand-crafting the generated code and using code generation to do some form of contract testing instead.

For example, the numberOrString stuff was added a long time ago and I wonder if we still actually need it?

The fieldReps code could definitely use some improvement. Currently it is just doing simple substring matching whereas ideally it would be doing tokenization or at least regex replacement. For example, currently it would change a field like catnip to catnIP. Ideally we could use a linter to ensure that our struct fields are named conventionally, which I believe is the motivation for doing the capitalisation in the first place.

The major issue I see with these customisations is that they reduce the utility of our generated code. If we can no longer rely on the generated code because someone needs to somehow verify that new fields don't require any customisations, then the utility is significantly diminished.

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

2 participants