-
Notifications
You must be signed in to change notification settings - Fork 26
use ((->) Unit) instead of Lazy #85
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
Conversation
safareli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on purescript/purescript-tailrec#18
package.json
Outdated
| "purescript-psa": "^0.5.0-rc.1", | ||
| "pulp": "^11.0.x", | ||
| "purescript": "0.11.x", | ||
| "purescript-psa": "^0.5.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are this changes fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by adding ps here we don't need to fetch it by hand in build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the core libraries we prefer to fetch by hand - it means that we can update libraries before the npm release is done.
src/Control/Monad/Trampoline.purs
Outdated
|
|
||
| -- | Suspend a computation by one step. | ||
| suspend :: forall a. Trampoline a -> Trampoline a | ||
| suspend = suspendF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand what's the point of suspendF and suspend. does it make sense for this implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove it since this is breaking anyway.
|
btw |
| - chmod a+x $HOME/purescript | ||
| - npm install -g bower | ||
| - npm install | ||
| script: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of two bower Installs here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second bower install will fetch devDependencies, splitting the build/test/install stuff this way ensures we're not accidentally relying on dev dependencies in the library.
|
This may already be in the works or may have already been decided; however, I am wondering if it would make sense to add a performance comparison to test the note in #83 that the implementation in this PR might be more efficient. |
|
@ethul I would need to update so:
|
|
Thanks!
…On Wed, Apr 26, 2017 at 04:44 Irakli Safareli ***@***.***> wrote:
@ethul <https://github.com/ethul> I would need to update /benchmark as
it's outdated. will try
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#85 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAVYy-MKDgqeH-X4wjZyq_wZgIjxb3Vuks5rzwPegaJpZM4NGfoE>
.
|
|
Is the Lazy/thunk even necessary? Why isn't the spine of |
|
FWIW, I'm not sure that |
|
It's useful to form cyclic structures, and I think some could be generated purely using |
|
What if we just remove it as there are already 3 ways to get trampoline? ( |
|
Uh, sorry, I was thinking of |
? |
fix #83