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 Schema elements as default values #12

Merged
merged 1 commit into from
Oct 31, 2019
Merged

Allow Schema elements as default values #12

merged 1 commit into from
Oct 31, 2019

Conversation

mabar
Copy link
Contributor

@mabar mabar commented Oct 12, 2019

  • new feature?
  • BC break? no

We have some composite extensions which simulates compiler passes. They can be disabled so configuration looks like this Expect::anyOf(false, SubExtension::createSchema()).

AnyOf does not match default value null (empty configuration) same way as if we directly use Expect::structure (no AnyOf) so that structure is not used.

Current workaround looks like this

	public function getConfigSchema(): Schema
	{
		$subExtensionSchema = SubExtension::createSchema();

		return Expect::structure([
			'subExtension' => Expect::anyOf(false, $subExtensionSchema)->default($subExtensionSchema->completeDefault(new Context())),
		]);
	}

With suggested change would be simplified call to default to default($subExtensionSchema) and it also allows any schemas used as default values.

Alternative solution would be match Structure with null value in AnyOf but I am not sure if it's a correct behavior as it would depend on order of structure and null if both used and maybe break also some other combinations.

@dg
Copy link
Member

dg commented Oct 22, 2019

Great. Can you add test?

@mabar
Copy link
Contributor Author

mabar commented Oct 22, 2019

Done

@dg
Copy link
Member

dg commented Oct 31, 2019

Thanks

@dg dg merged commit 554cb39 into nette:master Oct 31, 2019
dg pushed a commit that referenced this pull request Oct 31, 2019
dg pushed a commit that referenced this pull request Oct 31, 2019
dg pushed a commit that referenced this pull request Oct 31, 2019
@dg
Copy link
Member

dg commented Dec 31, 2020

Wouldn't it be better to use this solution, ie method that sets the first item as the default?

   Expect::anyOf(
        $subExtensionSchema
        false, 
   )->firstAsDefault(),

@mabar
Copy link
Contributor Author

mabar commented Dec 31, 2020

Imho it's better to keep that behavior hidden. If I implemented this now, I would throw exception for any Schema element except Structure, because behavior of inheriting e.g. null from Expect::string() is mostly confusing and useless.

But yeah, may be a nice shortcut.

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

Successfully merging this pull request may close these issues.

2 participants