Skip to content

Implements magic-do for Control.Monad.Effect#3289

Merged
kritzcreek merged 1 commit intopurescript:masterfrom
kritzcreek:magic-do-effect
Mar 27, 2018
Merged

Implements magic-do for Control.Monad.Effect#3289
kritzcreek merged 1 commit intopurescript:masterfrom
kritzcreek:magic-do-effect

Conversation

@kritzcreek
Copy link
Copy Markdown
Member

Simplest thing I could come up with. This adds another pass over the JS AST, so if that ends up being to slow I can look into merging these into a single pass, or checking for an import of Control.Monad.Eff/Control.Monad.Effect before doing the traversal.

@kritzcreek kritzcreek requested a review from garyb March 24, 2018 21:11
@garyb
Copy link
Copy Markdown
Member

garyb commented Mar 25, 2018

We're definitely not doing the rename to Effect just now then?

Copy link
Copy Markdown
Member

@garyb garyb left a comment

Choose a reason for hiding this comment

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

Changes look good though

@kritzcreek
Copy link
Copy Markdown
Member Author

It's easy enough to change in here if we decide to do it before 0.12, so I'll merge this and we can take a look at renaming the Control.Monad.Effect module to Effect later.

@kritzcreek kritzcreek merged commit 978c353 into purescript:master Mar 27, 2018
@kritzcreek kritzcreek deleted the magic-do-effect branch March 27, 2018 07:37
untilFixedPoint (return . tidyUp) . tco . inlineST =<< untilFixedPoint (return . magicDo) js'
untilFixedPoint (return . tidyUp) . tco . inlineST
=<< untilFixedPoint (return . magicDo')
=<< untilFixedPoint (return . magicDo) js'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know you already merged this, but since this is just a pure fn, could this not just be untilFixedPoint (return . magicDo' . magicDo) just to lose the extra fixed point pass (which can be expensive).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I wasn't entirely sure if it was safe to interleave the two transformations. In the rare case someone mixes the two in the same module, but I guess the typechecker would've rejected the program at that point...

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