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

Convert publish.js to Ramda functions #133

Merged
merged 5 commits into from
Oct 18, 2016

Conversation

MattMS
Copy link
Member

@MattMS MattMS commented Oct 10, 2016

No description provided.

)
),
R.flatten
)
Copy link
Member

Choose a reason for hiding this comment

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

Here's a simpler version:

//  simplifySee :: Array String -> Array String
var simplifySee =
R.pipe(R.chain(R.split(/\s*,\s*/)),
       R.map(R.replace(/^R[.]/, '')));

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, thanks!
Updated PR.

var prettifyCode = R.pipe(
R.join('\n'),
R.replace(/^[ ]{5}/gm, ''),
R.flip(R.invoker(2, 'highlight')('javascript'))(hljs),
Copy link
Member

Choose a reason for hiding this comment

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

I'd find this clearer:

function(s) { return hljs.highlight('javascript', s); }

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I did have mixed feelings about that code.
I've used a fat arrow instead, hope that's still fine.

Copy link
Member

Choose a reason for hiding this comment

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

If we're to use a fat arrow here, why not throughout the file? While we're at it we could replace occurrences of var with const. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Quick find-replace, no problems!

I had just wanted to make sure ES6 was fine first.

@davidchambers
Copy link
Member

I left one more comment, but this looks great!

@MattMS
Copy link
Member Author

MattMS commented Oct 17, 2016

Is this all good now?
I think I fixed that extra comment you made.

@buzzdecafe
Copy link
Member

i haven't found time to run it locally yet, but if it passed the gauntlet of @davidchambers review, i would expect it's good

@davidchambers
Copy link
Member

LGTM

@buzzdecafe
Copy link
Member

🐄

@buzzdecafe buzzdecafe merged commit af93043 into ramda:master Oct 18, 2016
@MattMS MattMS deleted the ramda_publish.js branch November 5, 2016 04:21
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