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

Avoid breaking catch/finally on angular promises #436

Closed
wants to merge 1 commit into from

Conversation

Marsup
Copy link
Contributor

@Marsup Marsup commented Mar 26, 2014

Fixes cases where code like :

$http(...)
  .then(...)
  .catch(...)
  .finally(...)

would end up looking like this :

$http(...)
  .then(...)
  .
  catch(...)
  .
  finally(...)

@bitwiseman
Copy link
Member

The current bug for this also includes throw... If you're going to do this it should work for all keywords, not as a special case for these few.

@zaggino
Copy link

zaggino commented Mar 26, 2014

+1
This also applies to many other codes, not only AngularJS ... most implemntations of http://promisesaplus.com/ use .catch and .finally even native ones:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
http://www.html5rocks.com/en/tutorials/es6/promises/

@Marsup
Copy link
Contributor Author

Marsup commented Mar 26, 2014

@bitwiseman The throw is now excluded from the scope and python version is done. Should I check around these tokens for context to make sure it's a promise or is it too much ?

@bitwiseman
Copy link
Member

See #368. I don't want take this for promises in particular. I want to see it work for any case where a keyword is used as the name of a property or function. You've done great work leveraging the existing structures in your other PRs. This one's a little harder because we want to treat any identifier that comes after . as a "non-keyword".

Since we do a bunch of string compares for keywords... The right thing is to add a flag.keyword, check it in all the places we do checks for keyword strings, and finally force it to false if the previous character is .. It'll be a wide change but a reasonable one.

@bitwiseman
Copy link
Member

This is what I'm talking about, although a bit rough...
bitwiseman@46676d2
I'm not in love with this exact implementation, but it is not horrendous. I'll translate to python later tonight.

@Marsup
Copy link
Contributor Author

Marsup commented Mar 27, 2014

Doesn't look rough to me, it actually feels very coherent with the coding style of the project.

@bitwiseman
Copy link
Member

Thanks. The change is what I described to you, it is in the right direction, and it is enough. TK_RESERVED still feels hacky to me. 😄

I'm going to close this in favor of mine. Thanks for the push to get this done - it is a heavily requested feature and unblocks us for correct handling of other ES6 and reserved words.

@bitwiseman bitwiseman closed this Mar 28, 2014
@Marsup
Copy link
Contributor Author

Marsup commented Mar 28, 2014

Well I can't take credit for it but thanks for supporting the effort of moving to an ES6-compatible version :)

@Marsup Marsup deleted the fix/angular-promises branch March 28, 2014 08:10
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.

3 participants