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

feat: header extraction improvement (close: #238) #271

Merged
merged 3 commits into from
Apr 29, 2018

Conversation

ulivz
Copy link
Member

@ulivz ulivz commented Apr 26, 2018

Summary

Fixed: #238
Test Header:

## `How` [It](/app/) <Works> **app** _italic_ *italic*

Snapshot

Before

image

After

image

.replace(/_(.*)_/, '$1') // _

// put here to avoid circular references
const compose = (...processors) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The checking of the processors.length seems unnecessary. Perhaps something like this?

const compose = (...processors) => {
  return processors.reduce((prev, next) => {
    return (...args) => next(prev(...args))
  }, x => x)
}

Or

const compose = (...processors) => {
  return [
    x => x,
    ...processors
  ].reduce((prev, next) => {
    return (...args) => next(prev(...args))
  })
}

Copy link
Member Author

@ulivz ulivz Apr 27, 2018

Choose a reason for hiding this comment

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

Hmmm? Are you telling me how to write code?

Code should be as easy as possible to understand, don't always think about drag racing.

If you expect me to make a change, first give redux a PR.

Copy link
Member

@yyx990803 yyx990803 Apr 27, 2018

Choose a reason for hiding this comment

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

@ulivz I also prefer easier to read code, but this is open source and @ycmjason is just offering what he thinks might be better code. He is not telling you how to write code and you don't have to be combative about this.

Copy link
Member Author

@ulivz ulivz Apr 27, 2018

Choose a reason for hiding this comment

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

@yyx990803 Yes, I should say convincing reasons instead of arguing for it.

@ycmjason I'm terribly sorry about my recklessness, and I liked the style what you said above, but, after a long time, I found I always need to think back to what parameters of reduce's callback needs to accept. Until one day I read the source code of Redux, I found it's easier to undestand. so I'd like this style.

Copy link
Contributor

@ycmjason ycmjason left a comment

Choose a reason for hiding this comment

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

Just one comment, LGTM otherwise.

@vuejs vuejs locked and limited conversation to collaborators Apr 27, 2018
@vuejs vuejs unlocked this conversation Apr 27, 2018

// put here to avoid circular references
const compose = (...processors) => {
if (processors.length === 0) input => input
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, did you miss return here? And the next line?

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! fixed at a8c6959

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