-
Notifications
You must be signed in to change notification settings - Fork 14
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 Frisby to an ES6 class #46
Conversation
paulmelnikow
commented
Apr 19, 2017
- Apply and enforce consistent indentation
- Switch a couple JSDoc comments to consistent format
- Consistent spacing between methods
- Apply and enforce consistent indentation - Switch a couple JSDoc comments to consistent format - Consistent spacing between methods
*/ | ||
addHeaders (headers) { | ||
const self = this; | ||
_.forEach(headers, function(val, key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought, but would it make sense to start using arrow functions for this upgrade as well to eliminate some of the const self = this
statements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be great! They'll make these things much more readable.
Since those changes are easy to get wrong, having more eyes to review them is good. It's impossible to review the diff here, so I would tend to make those in a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff is more contextual than I expected. Though I'd still feel more comfortable merging this first, and then tackling arrow functions.
Fine by me! Do whatever you like, I was just making a passing suggestion.
…On Wed, Apr 19, 2017, 2:08 PM Paul Melnikow ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/icedfrisby.js
<#46 (comment)>:
> - */
-Frisby.prototype.addHeaders = function(headers) {
- const self = this;
- _.forEach(headers, function(val, key) {
- self.addHeader(key, val);
- });
- return this;
-};
+ /**
+ * @param {object} headers - header object {k;v, k:v}
+ * @return {object}
+ * @desc Add group of HTTP headers together
+ */
+ addHeaders (headers) {
+ const self = this;
+ _.forEach(headers, function(val, key) {
This diff is more contextual than I expected. Though I'd still feel more
comfortable merging this first, and then tackling arrow functions.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#46 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEtMTNSohvPWUmv9BQHQIP1WShkxKNwvks5rxk2XgaJpZM4NBJ1->
.
|