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

Region close => empty #8

Open
jamesplease opened this issue Jun 19, 2014 · 6 comments
Open

Region close => empty #8

jamesplease opened this issue Jun 19, 2014 · 6 comments
Labels

Comments

@jamesplease
Copy link
Member

Region's close is empty, not destroy

@xMartin
Copy link

xMartin commented Jun 20, 2014

Yeah, the script wants to replace all regions' "close" with "destroy" which is hard to work around because views' "close" needs to be replaced with "destroy". Still, the proper replacement step for regions is completely missing.

@alxndr
Copy link

alxndr commented Aug 6, 2014

Still occurring

@jasonLaster
Copy link
Member

Maybe the proper fix is a message that warns you about this? The regex is pretty stupid.

@omasback
Copy link

+1. But I dont see how a better regex could fix this. The parser would need to know whether the instance being closed is a view or a region, which seems impossible to me. The tool would have to actually run the whole app and go back and check the prototype of each instance.

But there should definitely be a warning in the intro text to not change close to destroy for regions. And maybe an option to replace close with either destroy or empty on a case by case basis.

@infiniteluke
Copy link

Agree with @omasback. A warning message + choice for every close is probably best.

It took me 2 minutes to run the upgrade script on my app (Thank you!). I would imagine adding a little more verbosity wouldn't hurt anyone.

@jasonLaster
Copy link
Member

cool - this is ready for a PR.

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

No branches or pull requests

6 participants