Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Base class shouldn't necessarily be abstract #66

Open
carueda opened this issue Jun 28, 2020 · 5 comments
Open

Base class shouldn't necessarily be abstract #66

carueda opened this issue Jun 28, 2020 · 5 comments

Comments

@carueda
Copy link
Owner

carueda commented Jun 28, 2020

Related to #64, the base class shouldn't necessarily be abstract, but this is currently required per the issue 64b test, which refers to this spec:

#@define
BaseModelConfig {
  uuids: [string]
  scaling: double
}

#@define extends BaseModelConfig
LoadModelConfig {
  modelBehaviour: string
  reference: string
}

test.loadModelConfig = LoadModelConfig
@johanneshiry
Copy link
Collaborator

johanneshiry commented Jul 1, 2020

I dunno how much time I'll have within the next few weeks to work on this, but I agree that it might make sense to support either #@define or #@define abstract. One major change that would include #@define and allow its extension would be, that we cannot use case class anymore but we would have to go with class instead, because AFAIK case class cannot be extended.

This fact has been the main motivation to use the #@define abstract instead. I assume the logic will become far more complex if we have to check first if a specific #@define is extended (then is has to be a simple class) or not (then it can be a case class). Furthermore, having this in mind we would lose benefits provided by case classes for the simple class that is extended.

@carueda
Copy link
Owner Author

carueda commented Jul 1, 2020

Thanks @johanneshiry . Maybe it could be supported only for Java, at least initially. But no rush at all, I'll also try to find some time for this.

@carueda
Copy link
Owner Author

carueda commented Jan 2, 2025

Upon revisiting this, it appears that abstract classes in the generated code may be completely unnecessary. Given the core purpose of the tool —lib/app configuration access— tscfg could just merge the inheritance chain to directly generate the resulting Scala case class or Java record. This change would have no visible effect on the app or library, as configuration access would remain unchanged. Therefore, it should not introduce any breaking changes.(*)

So, the overall new handling would be:

  • @define abstract ... : no class generated

  • @define (no abstract): class generated as usual

  • @define extends ... : inheritance chain merged to generate resulting class.
    Such chain may involve both abstract and regular @defines.

No need for any changes in the input schema configuration file.


(*) The public API for the generated wrapper are the field names and structure of resulting associated types. That API would not change (despite internal differences in how the structures are constructed).

@carueda
Copy link
Owner Author

carueda commented Jan 2, 2025

So, for the example above:

#@define
BaseModelConfig {
  uuids: [string]
  scaling: double
}

#@define extends BaseModelConfig
LoadModelConfig {
  modelBehaviour: string
  reference: string
}

the following case classes would be generated:

case class BaseModelConfig(
    uuids: List[String],
    scaling: Double,
)

case class LoadModelConfig(
    uuids: List[String],
    scaling: Double,
    modelBehaviour: String,
    reference: String,
)

...

@carueda
Copy link
Owner Author

carueda commented Feb 12, 2025

To elaborate a bit about the planned change above about not generating any direct type for @define abstract entries in the schema (and invite any reactions/comments):

For client code, no source changes required at all, that is:

  • No changes needed in the input schema configuration file
  • No changes needed in configuration access using the generated wrapper

A "breaking" change would occur if existing client code happens to use any of the abstract classes previously generated, but, again, that use should not be considered part of the public API as mentioned previously.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants