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

Brace expansion should allow whitespace #16585

Closed
ahopkins opened this issue Apr 26, 2018 · 4 comments
Closed

Brace expansion should allow whitespace #16585

ahopkins opened this issue Apr 26, 2018 · 4 comments

Comments

@ahopkins
Copy link

Consider this example:

someWeatherComputedProperty: computed(
    'someObj.{ \
      day1.currentforecast, \
      day2.currentforecast, \
      day3.currentforecast, \
      day4.currentforecast, \
      day5.currentforecast, \
      day6.currentforecast, \
      day7.currentforecast}',
    'otherStuff',
    'moreOtherStuff', function() {

versus

someWeatherComputedProperty: computed('someObj{day1.currentforecast,day2.currentforecast,day3.currentforecast,day4.currentforecast,day5.currentforecast,day6.currentforecast,day7.currentforecast}', 'otherStuff', 'moreOtherStuff', function() {

Which one is easier to read?

Even the Error message itself makes the case:

Assertion Failed: Brace expanded properties cannot contain spaces, e.g. "user.{firstName, lastName}" should be "user.{firstName,lastName}"

I would suggest that user.{firstName, lastName} is simpler and more readable than user.{firstName,lastName}.

Am I missing something? Is there a legitimate reason that whitespace cannot be stripped out?

@Serabe
Copy link
Member

Serabe commented Apr 26, 2018

This is an intended behaviour. As such, changing it would require to follow the RFC process as describe in the CONTRIBUTING file

Thank you!

@Serabe Serabe closed this as completed Apr 26, 2018
@ahopkins
Copy link
Author

I understand that it is intended from the error message. I am more interested in knowing the why this is intended. Allowing whitespace allows for cleaner code.

@rwjblue
Copy link
Member

rwjblue commented Apr 27, 2018

Basically because we have to parse it at runtime, and dealing with fewer exception led to faster/easier code. Feel free to dig through the last couple rounds of refactoring the underlying code: #13231 and #15219.

Also, FWIW, your expansion can be simplified to:

computed('someObj.{day1,day2,day3,day4,day5,day6,day7}.currentforecast', function() {})

@ahopkins
Copy link
Author

Thanks for the links and for the heads up. 👍

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

3 participants