Should we flatten variables? about sourcery HOT 25 CLOSED

@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?

from sourcery.

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

from sourcery.

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

from sourcery.

cc @FilipZawada @vknabel @bejar37 @AliSoftware @kzaher

from sourcery.

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

from sourcery.

@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

from sourcery.

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)

from sourcery.

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

from sourcery.

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

from sourcery.

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

from sourcery.

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

from sourcery.

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)

from sourcery.

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.

from sourcery.

@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

from sourcery.

@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

from sourcery.

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

from sourcery.

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

from sourcery.

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

from sourcery.

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

from sourcery.

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)

from sourcery.

@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).

from sourcery.

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

from sourcery.

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?

from sourcery.

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

from sourcery.

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

from sourcery.