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

Builders should provide accessors to their fields #43

Closed
yjbanov opened this issue Dec 9, 2016 · 10 comments
Closed

Builders should provide accessors to their fields #43

yjbanov opened this issue Dec 9, 2016 · 10 comments
Assignees

Comments

@yjbanov
Copy link
Contributor

yjbanov commented Dec 9, 2016

For example, imagine I was to construct a factory that forwards its parameters to a constructor:

class Foo {
  factory Foo(A a, B b, C c) {
    return new Foo._(a, b, c);
  }

  Foo._(A a, B b, C c);
}

To generate factory Foo(A a, B b, C c) I will have a List<ParameterBuilder>. To forward the parameters to Foo._, I want to be able to convert List<ParameterBuilder> to a List<ExpressionBuilder> args and pass that to myType.newInstance(args).

Example:

List<ExpressionBuilder> forwardParamsToArgs(List<ParameterBuilder> params) {
  return params.map<ExpressionBuilder>((p) => reference(p.name)).toList();
}

Unfortunately p.name does not exist.

@matanlurey
Copy link
Contributor

That's a good fundamental question. It's not impossible to do, certainly something like name is easy.

@alorenzen and @natebosch, WDUT?

@natebosch
Copy link
Member

I'm curious how often this comes up.

When you create the List<ParameterBuilder> can you create the List<ExpressionBuilder> alongside it?

I could get on board with the idea, though I think in many cases it might be better to go data -> builder1,builder2 than data -> builder1 -> builder2

@zoechi
Copy link
Contributor

zoechi commented Dec 10, 2016

Is this similar/related to #40?

@yjbanov
Copy link
Contributor Author

yjbanov commented Dec 12, 2016

@natebosch Yeah, creating the two lists (or more, if, e.g., you also want to generate class fields) is a solution. However, it either requires tighter coupling or additional custom data structures for storing information about parameters/fields. A simple example:

var params = data.where(_accept).map<ParameterBuilder>(_createParam).toList();
var args = params.where(_pass).map<ExpressionBuilder>(_createArg).toList();

But since I can't use params, I have to save the results of the first line in something else, which seems redundant as ParameterBuilder is already a perfectly fine object for storing parameter information:

class MyParam { var type; var name; }

var paramData = data.where(_accept).map<MyParam>(_createParamData).toList();
var params = paramData.map<ParameterBuilder>(_createParam).toList();
var args = paramData.where(_pass).map<ExpressionBuilder>(_createArg).toList();

Or avoid list comprehensions and instead run a single loop that couples the two constructions.

@natebosch
Copy link
Member

I can see your point. If he had nice Tuples or something this would be easier, but having to define a class is a lot of overhead. I think this feature is worth adding.

Side note: In your examples all of the generic arguments to .map aren't necessary as long as the _create* methods are well typed.

@alorenzen
Copy link
Contributor

Yeah, I can definitely see some value to this.

@matanlurey
Copy link
Contributor

Alright, SGTM.

@matanlurey matanlurey changed the title Should builders provide accessors to their fields? Builders should provide accessors to their fields Dec 13, 2016
@matanlurey
Copy link
Contributor

I'm going to add ParameterBuilder#name, but others will have to be added as-needed.

matanlurey added a commit to matanlurey/code_builder that referenced this issue Dec 25, 2016
matanlurey added a commit that referenced this issue Jan 5, 2017
* Add for/for-in loop

Closes #49

* Add support for while/do-while

* Add constructor initializers

Closes #50

* Update CHANGELOG and pubspec for beta

* Add “name” getter for ParameterBuilder

Partial support towards #43

* Update CHANGELOG

* Address feedback
@yjbanov
Copy link
Contributor Author

yjbanov commented Jan 10, 2017

Aye!

@matanlurey
Copy link
Contributor

Closing for now, we can re-open for specific needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants