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

Allow callable in args #1492

Closed
wants to merge 1 commit into from
Closed

Allow callable in args #1492

wants to merge 1 commit into from

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Dec 5, 2023

This makes it possible to dynamically alter the args via a callable.

@ruudk
Copy link
Contributor Author

ruudk commented Dec 5, 2023

@spawnia Ready for review, whenever you have time 💙

@spawnia
Copy link
Collaborator

spawnia commented Dec 5, 2023

This makes it possible to dynamically alter the args via a callable.

I don't understand the benefit for that, please elaborate.

On the performance side, I only see this hurting. Given that Argument::listFromConfig() is called eagerly, there is no bonus for lazyness to be had here.

@ruudk
Copy link
Contributor Author

ruudk commented Dec 5, 2023

I agree that there is no performance benefit of doing this lazily.

But it allows for using yield to return arguments that are depending on conditions.

Example:

fn() {
  yield 'id' => ['type' => Type::int()];
  if ($this->someFeatureFlag()) {
    yield 'other' => ['type' => Type::int()];
  }
  if ($this->someSpecialFlag()) {
    yield 'special' => ['type' => Type::int()];
  }
}

Obviously, we can also generate this array before constructing the type, or we can use a method that returns the flat array.
But everywhere we allow for callable that can yield results, except here.

@spawnia
Copy link
Collaborator

spawnia commented Dec 5, 2023

But everywhere we allow for callable that can yield results, except here.

Disagree, we only do that when it brings performance benefits. Other config options don't allow it, e.g. name.

I think it is somewhat misleading to offer this option since in other cases lazyness helps performance. In this case, it hurts performance - whether it is used or not. Thank you for the suggestion, but I am going to pass on this.

@spawnia spawnia closed this Dec 5, 2023
@ruudk ruudk deleted the args-callable branch December 5, 2023 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants