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

undefined behavior for nested brace expansion #13693

Closed
kellyselden opened this issue Jun 16, 2016 · 2 comments
Closed

undefined behavior for nested brace expansion #13693

kellyselden opened this issue Jun 16, 2016 · 2 comments

Comments

@kellyselden
Copy link
Member

I've come across some code that could use nested brace expansion. I tried it out manually first and got this:

Ember.expandProperties('first.{second1,second2.{third1,third2}}', echo);
first.second1third1
first.second1third2
first.second2.third1
first.second2.third2

I've read this which hints that it was discussed or decided to not support. If it is unsupported, perhaps it should error, like when you put a space in. Or maybe it just needs a regex update to fully support it? It could make computeds further collapsable.

@pixelhandler
Copy link
Contributor

@kellyselden do you know if there are any tests for "nested" brace expansion in the project?

I only found this one: https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/tests/computed_test.js#L461-L482

Also are there any docs on nested brace expansion? I found https://guides.emberjs.com/v2.6.0/object-model/computed-properties/#toc_computed-properties-in-action

My thoughts are that to support "nested brace expansion" we'd need some tests and docs. Maybe the way is to answer the question "Is it supported?" is to start with adding a test that clearly makes it unsupported, since there are no tests or docs nesting.

However, that does seem like an edge case. If there are no docs or tests that indicated you could use "nested brace expansion". Even simpler - update in the guides to indicate that nesting braces in a computed property is not supported.

Alternatively - like using spaces is not supported with brace expansion, https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/tests/computed_test.js#L484-L491 It may be a good idea to have a test and assertion that a computed property cannot use "nested brace expansion".

@pixelhandler
Copy link
Contributor

@rwjblue @locks is there any undocumented consensus on supporting nesting in brace expansion?

Serabe added a commit to Serabe/ember.js that referenced this issue Nov 4, 2016
In emberjs#13693, it is asked to error for an unsupported feature. Though we
don't say that this is not supported, it would be nice if we could let
the user know that what he is trying to do will not work instead of
silently fail.

Fixes emberjs#13693
Serabe added a commit to Serabe/ember.js that referenced this issue Nov 4, 2016
In emberjs#13693, it is asked to error for an unsupported feature. Though we
don't say that this is not supported, it would be nice if we could let
the user know that what he is trying to do will not work instead of
silently fail.

Fixes emberjs#13693
Serabe added a commit to Serabe/ember.js that referenced this issue Nov 4, 2016
In emberjs#13693, it is asked to error for an unsupported feature. Though we
don't say that this is not supported, it would be nice if we could let
the user know that what he is trying to do will not work instead of
silently fail.

Fixes emberjs#13693
rwjblue pushed a commit that referenced this issue Nov 7, 2016
In #13693, it is asked to error for an unsupported feature. Though we
don't say that this is not supported, it would be nice if we could let
the user know that what he is trying to do will not work instead of
silently fail.

Fixes #13693

(cherry picked from commit 6f2d615)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants