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

Update qs dependency to 6.3.0 #215

Closed
wants to merge 6 commits into from
Closed

Update qs dependency to 6.3.0 #215

wants to merge 6 commits into from

Conversation

ysangkok
Copy link

@ysangkok ysangkok commented Dec 21, 2016

The API has not significantly.

Changelog here: https://github.com/ljharb/qs/blob/master/CHANGELOG.md#630

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Thanks for the help, @ysangkok ! Most of the work of updating a dependency is in getting our HISTORY.md file updated, to reflect the changes the user will experience with the updated dependency (only changes they'll experience, not all changes). If you can update this PR with that information that would be super awesome, as right now the PR isn't anything on top of what greenkeeper.io does.

@ysangkok
Copy link
Author

@dougwilson OK, please tell me if it is OK now.

@dougwilson
Copy link
Contributor

Please keep the PR to only one change. I also still don't see any description of that users will experience from the dep update in the history file.

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Too many changes for a single PR; missing summary of changes in history.

@dougwilson
Copy link
Contributor

Sorry I don't have docs on what we expect; historically we have not accepted these kind of changes if it was simply to bump a dep. I will try to come up with some docs over this holiday season. No ETA on that or a merge, though.

@ysangkok
Copy link
Author

@dougwilson So, I reverted the other deps upgrades and added details on the changes I deem most important (new features added). The other changes are bug fixes or optimization, or refactorings that shouldn't be visible to users. Please tell me if this is in a mergeable state. I do not care much when this is merged, I just want the pull request to be perfect. Thanks in advance.

@dougwilson
Copy link
Contributor

Great, thanks! Can you remove the links and keep the descriptions concise? Use the other qs upgrades in the history file as an example. I am on and off planes, so cannot give more specific guidance for some time.

@@ -1,3 +1,11 @@
1.15.3 / 2016-12-xx
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just be "unreleased" you can use past commits to see how that is done until we have docs on the process.

===================

* deps: qs@6.3.0
- Date stringification format configurable (https://github.com/ljharb/qs/issues/159)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use the stringification API, so this is an irrelevant change to our users. Please only list the changes that affect our users.


* deps: qs@6.3.0
- Date stringification format configurable (https://github.com/ljharb/qs/issues/159)
- RFC 1738 support (`+` instead of `%20`). Must be explicitly enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a change in parsing or stringification?

* deps: qs@6.3.0
- Date stringification format configurable (https://github.com/ljharb/qs/issues/159)
- RFC 1738 support (`+` instead of `%20`). Must be explicitly enabled.
- For remaining changes, see [qs/CHANGELOG.md](https://github.com/ljharb/qs/blob/master/CHANGELOG.md#630)
Copy link
Contributor

Choose a reason for hiding this comment

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

So the purpose of adding the information here is specifically so no one ever has to go reference the dependency change log :) I would remove this.

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Changes from 6.2.1 are missing from the list here.

@ysangkok
Copy link
Author

ysangkok commented Dec 22, 2016

@dougwilson The first two changes I listed here are not relevant for the library consumer. In fact, as far as I can see, the only change that change the result of functions is this one: [Fix] ensure key[]=x&key[]&key[]=y results in 3, not 2, values (from 6.2.1). Other changes are performance related. The other [Fix] changes the value of qs.parse('foo[]=bar&foo[][first]=123&foo[second]=456'). Do you think it would be appropriate to mention those two fixes and nothing else?

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.

None yet

2 participants