Skip to content

Introduce --codegen argument so targets can be specified explicitly#3324

Merged
garyb merged 6 commits into
purescript:masterfrom
garyb:customisable-codegen
Apr 27, 2018
Merged

Introduce --codegen argument so targets can be specified explicitly#3324
garyb merged 6 commits into
purescript:masterfrom
garyb:customisable-codegen

Conversation

@garyb

@garyb garyb commented Apr 26, 2018

Copy link
Copy Markdown
Member

Resolves #3196, closes #3258

Default is js, using sourcemaps implies js and the separate option for source maps is gone now.

Thanks to @gabejohnson for doing the first part of this!

@garyb

garyb commented Apr 27, 2018

Copy link
Copy Markdown
Member Author

Any thoughts/comments about this @kritzcreek @LiamGoodacre @hdgarrood?

@hdgarrood

Copy link
Copy Markdown
Contributor

👍 I like this a lot!

@kritzcreek kritzcreek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a minor question

Comment thread app/Command/Compile.hs
targetParser :: Opts.ReadM [P.CodegenTarget]
targetParser =
Opts.str >>= \s ->
for (T.split (== ',') s)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to be a bit more lenient here and strip the text after splitting?

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'm not sure how you'd get spaces into that value anyway? Since --codegen js, corefn wouldn't parse as one thing.

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.

Oh... quotes maybe?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, that's what I'd expect to happen

@LiamGoodacre

Copy link
Copy Markdown
Member

Awesome. No comments other than what @kritzcreek raised. 💯

Comment thread app/Command/Compile.hs Outdated
$ maybe (Opts.readerError targetsMessage) pure
. flip M.lookup targets
. T.unpack
. T.takeWhile (/= ' ')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Thanks, couldn't find that when I looked through the docs before!

@garyb garyb force-pushed the customisable-codegen branch from 99547dd to c40b016 Compare April 27, 2018 15:28
@garyb garyb merged commit 91e77b7 into purescript:master Apr 27, 2018
@garyb garyb deleted the customisable-codegen branch April 27, 2018 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add flag to disable Javascript backend

5 participants