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

How will YAML change as we add more languages? #289

Open
liamappelbe opened this issue Mar 1, 2022 · 11 comments
Open

How will YAML change as we add more languages? #289

liamappelbe opened this issue Mar 1, 2022 · 11 comments

Comments

@liamappelbe
Copy link
Contributor

liamappelbe commented Mar 1, 2022

We need to figure out how to adapt the YAML config as we add more languages (eg ObjC, Java, Swift, Kotlin).

For the input source files, for Java/Kotlin/Swift we can just look at the file extension, but ObjC also uses .h files. The easiest solution would just be to add an objc-headers key, at the same level as headers, and I think that would also make sense for the other languages.

The main issue I see is that most of the other top level config keys are either not applicable to some languages (eg structs/typedefs/unions/macros), or we'd want them separated by language (eg functions might have different naming conventions in different languages, so the regexes would look different).

Should we just add a bunch more top level keys, but with language specific prefixes? Eg objc-functions, java-classes? Or maybe we should totally restructure the format so that the language is at the top level:

ffigen:
  output:
  c:
    headers:
    functions:
    ...
  objc:
    headers:
    classes:
    ...
@mannprerak2
Copy link
Contributor

How about adding a top level lang key to the config.

#  pubspec.yaml
ffigen:
  lang: 'c'
  headers: ...
  functions: ...

This still doesn't provide a way to specify multiple configurations in a single file, but that seems rare since bindings would usually be abstracted away as packages. And one can always specify multiple config files in that case. (Each for a different language/binding)

@dcharkes
Copy link
Collaborator

dcharkes commented Mar 2, 2022

We already have multiple config files in case of conceptually separate binding sets in one language, see for example 2 ffigen.yamls in:

If we'd want to support generating multiple binding sets from a single config, we should maybe also allow that for multiple times the same language.

@liamappelbe do you have a use case where you'd want two languages in one config?

Both yaml designs seem fine to me for supporting different keys for different languages.

  • The nested in language ensures yaml keys are valid purely based on nesting, and not on sibling values. Which conceptually makes sense (and maybe makes validation easier).
  • The sibling lang key communicates better that there can be only one language in the config.

@liamappelbe
Copy link
Contributor Author

I don't have a use case where we'd need multiple languages in one package, but maybe it'll come up when we support both ObjC and Swift. In any case, if we can have multiple config files, then one for each language, with a lang key, sounds reasonable to me. The only issue I see is that each language is only going to support a subset of the other fields. I suppose that's mostly just a documentation and validation issue though.

@liamappelbe
Copy link
Contributor Author

@mannprerak2 @dcharkes After Prerak's comment on dart-archive/ffigen#313, I think that methods need a different approach. Similar to how member-rename allows renaming a member of a specific struct, I think users are going to want to be able to include/exclude methods of specific interfaces. So having a top level objcMethods config isn't a good idea, as it would apply to all methods in all interfaces. I'm thinking of doing something like this instead:

objcInterfaces:
  member-rename:
    'NSString':
      ...
  member-include:
    'NSString':
      - '.*'
  member-exclude:
    'NSString':
      - 'length'

So member-rename applies to interface methods the same way it applies to struct members. And member-include/exclude have the same sort of string keys as member-rename, but full include/exclude lists as values.

I might try to land dart-archive/ffigen#313 with just the objcInterfaces config key for now, and just remove the objcMethods config, until we figure out the details of how to hand methods.

@dcharkes
Copy link
Collaborator

dcharkes commented Apr 13, 2022

That does make a lot of sense indeed. Methods are nested in interfaces/classes in the same way fields are nested in structs.

Given that the member rename for structs is structs -> member-rename -> [struct match] -> [member match] : [new name], we can follow the same pattern as there as you suggested.

structs:
  rename:
    # Removes prefix underscores
    # from all structures.
    '_(.*)': '$1'
  member-rename:
    '.*': # Matches any struct.
      # Removes prefix underscores
      # from members.
      '_(.*)': '$1'

(And yes, then we can have include/exclude members as you suggest, which don't make sense for structs, but do for methods in interfaces.)

I'm wondering if it is worth considering changing up the nesting to: structs -> [struct match] -> member-rename -> [member match] : [new name]. That would keep all configuration for a single struct, or in this case a single objectiveC interface closer together. cc @mannprerak2, can you remember why we chose the nesting we did for structs? One problem with reversing the order is that rename is at the same level as the [struct match].

@liamappelbe wdyt about the nesting order for the config?

@mannprerak2
Copy link
Contributor

The nesting order was mostly because we have regexp matching support. So if one wants to do a member rename across all structs, that would be possible (commonly used for removing the prefix underscore)

@liamappelbe
Copy link
Contributor Author

I think nesting rename, include, and exclude under [struct-match] makes logical sense. This would also allow us to unify the Renamer and MemberRenamer, and reuse the Includer rather than building a new MemberIncluder.

We'd still be able to do regexp matching on the structs.

@dcharkes
Copy link
Collaborator

dcharkes commented Apr 13, 2022

It would be a breaking change for structs API. Though I'm fine with that if we give people a nice migration message on the old syntax.

It is possibly best to do the breaking change for structs first before landing objInterfaces, to keep things consistent.

@mannprerak2
Copy link
Contributor

It is possibly best to do the breaking change for structs first before landing objInterfaces, to keep things consistent.

So are we planning remove the nesting in config then?

@dcharkes
Copy link
Collaborator

I was thinking something like:

structs:
  rename:
    # Removes prefix underscores
    # from all structures.
    '_(.*)': '$1'
  '.*': # Matches any struct.
    member-rename:
      # Removes prefix underscores
      # from members.
      '_(.*)': '$1'

Of course, that means we cannot match a struct called rename anymore. Or did you have something different in mind @liamappelbe

@liamappelbe
Copy link
Contributor Author

Yeah, that was what I was thinking. But I hadn't considered that we wouldn't be able to match a rename struct. Tbh, I don't know enough about YAML to know when it decides what is a string and what isn't (I thought it was the quotes).

Maybe the rename limitation doesn't matter too much, because in the unlikely event that this happens, they would get an error because the stuff nested under the rename key would have the wrong structure. It wouldn't silently do unexpected stuff. And in that case they could switch to a regex like '[r]ename':.

@liamappelbe liamappelbe transferred this issue from dart-archive/ffigen Nov 15, 2023
HosseinYousefi added a commit that referenced this issue Nov 16, 2023
… exported classes (#289)

* Removes the support for importMap in config.
* Adds the support for importing a yaml symbols file.
HosseinYousefi added a commit that referenced this issue Nov 16, 2023
… exported classes (#289)

* Removes the support for importMap in config.
* Adds the support for importing a yaml symbols file.
parlough pushed a commit to parlough/native that referenced this issue Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants