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

Implement v2 query string migrate #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alfa-jpn
Copy link

description

This PR is for query parser/build breaking chage (Mithril.js#2025).

implements

  • Add migrate/v2
    • Implement v1 parseQueryString and buildQueryString
  • Add tests
    • npm test

@alfa-jpn alfa-jpn changed the title Implement v2 query string migrate [WIP] Implement v2 query string migrate Dec 10, 2017
Copy link
Owner

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

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

Looks good, but you need to indent everything by 4 spaces, not two. (That's what everything else uses.)

I do appreciate the tests, though.

@dead-claudia
Copy link
Owner

Also, note: I'm not going to merge this until the corresponding Mithril PR is.

@alfa-jpn
Copy link
Author

alfa-jpn commented Dec 17, 2017

@isiahmeadows
Thanks for the review.
I fixed the indent problem, and I added eslint to prevent recurrence.

best regards,

Copy link
Owner

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

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

So far, the only request remaining is that you remove the ESLint stuff, since I'm going to be pushing something different (to bring it in line with my other projects).

@alfa-jpn
Copy link
Author

@isiahmeadows
I see. I removed the linter. 🙇‍♂️

best regards,

@alfa-jpn alfa-jpn changed the title [WIP] Implement v2 query string migrate Implement v2 query string migrate Jan 28, 2018
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.

2 participants