openfga / language

Grammar for the OpenFGA modeling language
https://openfga.dev
Apache License 2.0
15 stars 7 forks source link

feat: allow passing a custom schema version to modular transform #193

Closed ewanharris closed 3 months ago

ewanharris commented 3 months ago

Description

Adds support for passing the schema version when transforming a modular model.

Initially I was going to do this in an optional manner, but then realised that realistically anyone who is using this will always pass the schema version anyway so it made sense to move this to required and not have a default version.

Validation is left to the standard validation logic.

References

Closes #188

Review Checklist

ewanharris commented 3 months ago

I wasn't 100% decided on what a right signature would be for Go as I think the catchall options object pattern isn't a common pattern like it is in JS.

I initially had it implemented using the Options pattern but with making it a required parameter that didn't really work so I went with the parameter. Then most likely in future if we need to add more parameters we could add them using the options pattern with defaults.

I can switch to accepting a struct that copies the JS options if we'd just prefer consistency across the packages?

rhamzeh commented 3 months ago

Hmm.. In that case we can make js like Go?

I'm also jot sure maybe schemaVersion should be the first param?

ewanharris commented 3 months ago

@rhamzeh a7dd6e8f61b7afb8789353fabee61aadde20b46e aligns the JS function arguments with Go, I'd usually be a bit meh on that because it might not be obvious what the second argument is but realistically most usage will be like transformModuleFilesToModel(files, fgaMod.schema.value) so realistically the "magic argument" doesn't really apply. We can apply the JS options catchall and the Go options pattern to both of these functions in future with no breaking changes too so I think we're fine

I do think that files should be the first parameter as the function should be read like transform these files with schema version X imo