Should we flatten variables?

[krzysztofzablocki]

If a type inherits from another type, or implements given protocol, should the variables be available in the given type itself?
e.g.

protocol Something {
  var typeName: String { get }
}

struct Foo: Something {
}

1. Should we list typeName in Foo? if so should we add alternative variable so people can choose between inherited or direct variables?

2. Also same question can be asked in regards to annotations, if the protocol specified type level annotation, should we flatten that into Foo ?

@ilyapuchka what do you think?

[krzysztofzablocki]

cc @FilipZawada @vknabel @bejar37 @AliSoftware @kzaher

[ilyapuchka]

Absolutely. But not sure what do you mean by "choose between inherited or direct variables"?

[krzysztofzablocki]

@ilyapuchka on both questions? what I meant is that it might be useful to be able to differentiate between variables defined in the class vs supertype

[ilyapuchka]

Regarding annotation, I don't think we should propagate them, anyway it's easy to add annotations.
Regarding distinguishing inherited properties from own properties, how that can be used?
Another question to add to the list - should we combine annotations when overriding property or method or just override them? (I would say we should override)

[krzysztofzablocki]

override makes sense to me, I'll skip differentiation between inherited vs own for now since we don't have use-case

[krzysztofzablocki]

@ilyapuchka shouldn't we do the same with methods then? I think the way you implemented it now it won't inherit from supertype?

[ilyapuchka]

Sure. Currently it only adds methods from extensions, the same as variables.

[vknabel]

I think flattening all variables by default could lead to duplicate code generation, but a flattened context could still be very useful

[krzysztofzablocki]

@ilyapuchka @vknabel so what I've right now is:

• variables - all available variables = typeVariable + variables from inherits and implements
• typeVariables (could use a better name?) - only variables defined in this type (or it's extension)

All our filters e.g. staticVariables apply against variables now, should we modify this behaviour?

maybe leave variables as is on master and name the new property allVariables / flatVariables + corresponding allStaticVariables but maybe you have better name suggestions?

[AliSoftware]

I think we shouldn't flatten the variables by default but offer a special variable (like super or similar) which exposes the flatten representation.

So Foo.variables wouldn't contain typeName but Foo.super.variables would (or maybe inherited instead of super, or ^ ? I'm open to alternatives)

[krzysztofzablocki]

@AliSoftware how about the suggestion I added above? keeping variables as is and adding [all or flattened]Variables and corresponding filters

[ilyapuchka]

Accessing super class variables through supertype is fine, but what about i.e. computed variables (plus static) or methods defined in protocol extensions?
Having variables and allVariables (the same pattern applied to methods) looks like the best option.

[AliSoftware]

@krzysztofzablocki I prefer all.variables / flatten.variables rather than allVariables / flattenVariables, feels more flexible and generic to me, and a little similar to your implementing.Foo / inherit.Foo / … other patterns imho

[AliSoftware]

@krzysztofzablocki so basically we have the same idea (keeping variables unchanged and adding a separate entry for flatten Variables) except that I would give that new entry a name with dot, while you propose a camelcase one

[krzysztofzablocki]

Doesn't type.all.variables sound a little weird?

[AliSoftware]

Hence me being open to alternate names. Like type.inherited.variables maybe?

[krzysztofzablocki]

I think it only make sense to have distinction between local and all variables, thus type.inherited.variables would be confusing.

if we don't want to use allVariables then how about doing something like this:

type.variables.local <- local variables to this type
type.variables.local.instance <- local instance variables
type.variables.all <- local + inherited / implementing
type.variables.all.instance <- all instance variables
etc.

Happy to replace [all, local] with better naming suggestion

[krzysztofzablocki]

@vknabel @AliSoftware @FilipZawada @ilyapuchka ^ thoughts? I've implemented this already so I just need us to decide on naming

[AliSoftware]

That means that type.variables won't be valid anymore (I mean, as a value that would be rendered by Stencil directly). It won't be an array on which we could {% for v in type.variables %} directly then? Wondering if that wouldn't be confusing? (But not sure I like any solution anyway)

[vknabel]

@AliSoftware's point about compatibility is quite strong and I think all.variables reads quite well.

Alternatively: does it make sense (if possible) to use Stencil filters for this? Like {% for v in type.variables | local %}. On the downside it requires all (or flattened) to be the default and it requires variables to contain their origin (inherited vs local).

[krzysztofzablocki]

I'm adding allVariables and introducing few filters as I liked that suggestion from @vknabel 🥇

I've also removed documentation for staticVariables, computedVariables, instanceVariables but they will still work until 1.0, we should be moving to filters

[ilyapuchka]

With filters, how do we get the number of static/computed/other variables if we remove dedicated collections for variables?

[krzysztofzablocki]

Good question, I think Stencil doesn't support () so .count won't work but once we have set node from @AliSoftware we can set variable to filtered array.

{% if type.allVariables|instance %} should work as well as it will run if statement around empty array?

[krzysztofzablocki]

@ilyapuchka I added count filter that will turn array into its count and since we can chain filters that will work

[ilyapuchka]

Cool, looks pretty elegant solution! Will look closer at your pr later today