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

Add ->* and yield #462

Closed
wants to merge 3 commits into from
Closed

Add ->* and yield #462

wants to merge 3 commits into from

Conversation

vendethiel
Copy link
Contributor

"wip"

Adds a basic version of generators. Tests courtesy of @alubbe.

What's (could be) missing :

  • yield from
  • tests for an edge case like ~~>*
  • disable class => ->* (currently has an error, fixed on my local repo) (also for !->* if we go the hushed route)

Questions :

  • Is this syntax okay ? (same for function)
  • Should these be hushed by default ? (they're here)

@@ -1623,6 +1623,10 @@ class exports.Fun extends Node
o.indent += TAB
{body, name, tab} = this
code = \function
if @generator
if @ctor
@ctor.error "A constructor can't be a generator"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(fixed in my local repo)

@hmaurer
Copy link

hmaurer commented Jan 30, 2014

🍰

@wryk
Copy link

wryk commented Jan 30, 2014

🍰 WONDERFUL DAY

@askucher
Copy link

Well done!

On Fri, Jan 31, 2014 at 12:19 AM, Mathieu Gallé-Tessonneau <
notifications@github.com> wrote:

[image: 🍰] WONDERFUL DAY

Reply to this email directly or view it on GitHubhttps://github.com//pull/462#issuecomment-33740649
.

@vendethiel
Copy link
Contributor Author

I'll try to get travis setup correctly tomorrow

@gkz
Copy link
Owner

gkz commented Jan 31, 2014

Awesome! Thanks for your work :)

The syntax looks good.
And yes, they should be hushed by default.

Once this gets merged in I'll look into releasing 1.3.

@vendethiel
Copy link
Contributor Author

Mostly ready. Needs a special setting for travis -- @gkz do you know how ?

Didn't add function* because of how it's currently parsed.

@vendethiel
Copy link
Contributor Author

@gkz There doesn't seem to be a way for travis to run a certain command only on a certain platform (that I've found). Should be okay other than that.

@duralog
Copy link

duralog commented Feb 4, 2014

hmm, you're right, which is why I want to think about it more. I don't want to remove functionality from the generators, just to make them easier on the eyes for basic patterns. I'll have a clearer idea in a few days, as I'm still getting used to to new concepts.


I think, if you put engine node: ">= 0.11.3" it will run the tests on node with harmony (like so: https://github.com/cojs/archan/blob/master/Makefile), but, I think npm will give you a warning if you try to install on 0.10.x so that's not optimal.
instead, I think the solution to that is to add all engines to travis:

 node_js:
   - "0.4"
   - "0.6"
   - "0.8"
   - "0.10"
   - "0.11"

and then in the test script just do a conditional which checks the node version and if it's 0.11.x run the harmony tests as well. I recommend separating these tests out into a separate file and spawning a node process with --harmony-generators from inside the test script.

I'm pretty sure I can get it working. do you want me to make a PR?

@gkz
Copy link
Owner

gkz commented Feb 4, 2014

Thinking about the ->* and function* syntax, I think it might be a better idea to add the use of * when initially parsing, rather than parsing * seperately. This would mean adding * to the arrow regex and adding function* as a keyword.

@vendethiel
Copy link
Contributor Author

I started off with that, but it would double code here : https://github.com/gkz/LiveScript/blob/master/src/lexer.ls#L593

@duralog
Copy link

duralog commented Feb 4, 2014

whatever... it's just a test. I think it's actually better to do the whole test suite utilizing with both harmony enabled and disabled (make sure harmony doesn't break something).

if not process.env.HARMONY_ENABLED and process.versions.node.substr(0,4) is \0.10
  spawn \node, [\--harmony, __filename], env: process.env <<< HARMONY_ENABLED: \yep
# buncha tests...
if process.env.HARMONY_ENABLED
  require './test.harmony'

@igl
Copy link
Contributor

igl commented Feb 4, 2014

Isn't it possible to add the * automatically when yield is used in the function body?

@duralog
Copy link

duralog commented Feb 5, 2014

yes, it is, but you wouldn't want that in some cases. imagine this:

fs.readFile 'package.json', (err, data) ->
  do_something
  lala = yield something
  do_something_else lala

which if you combined this with a package like co it might be useful... it could turn into:

fs.readFile 'package.json', (err, data) ->
  do_something
  (co ->*
    lala = yield something
  )(err, lala) ->
    if err then throw err
    do_something_else lala

which is sorta the stuff I was thinking about for making the language better
bueno, at least it should give a better error saying that you are using yield in an unstarred function.

@off-by-some
Copy link

I request we rename LiveScript to "Arrow"

@gkz
Copy link
Owner

gkz commented Feb 5, 2014

Let's work on branch:
https://github.com/gkz/LiveScript/tree/generators

@naturalethic
Copy link

How about:
foo* = func()
for:
foo = yield func()

@naturalethic
Copy link

Or even:
foo = *func()

@naturalethic
Copy link

So who's gonna fork LiveScript to presume every function is a generator? Does that even make sense?

This was referenced Feb 13, 2014
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.

None yet

9 participants