fix the way http verbs are defined

[samjross]

I changed the way they are defined so that they are more IDE-friendly. The way they were defined, an IDE wouldn't easily recognize them and would often show 'request.get' or 'request.post' etc as a possible error. Ctrl+clicking on the 'get' function wouldn't take you to where it was defined. Now it will!

[samjross]

looks like I messed up on formatting. sorry.

[simov]

@flanneljesus I think that's a problem with your IDE. I don't think we should make code changes just to please someones code editor. Interestingly enough your IDE didn't showed you the eslint errors.

[samjross]

would any IDE recognize things defined like that? I would be surprised; adding object properties in that method...that's how you add dynamically defined properties. Does your IDE recognize that?

The linter settings for this project are very different from the default in my IDE (webstorm for what it's worth) and I don't know where to find the settings for this project.

[samjross]

Ah, I found the eslint file in the root of the repo. Sorry for this, I'm new to making pull requests to other people's repos. Not trying to be an idiot.

I would genuinely like to know if other IDEs recognize 'request.post' though.

[samjross]

I added a new commit with the formatting fixed according to the package eslint file.

[froatsnook]

@flanneljesus You are, by far, not the only person to miss the eslint and make a failing pull request 😆. Did you run npm test? Looking for and running the test suite are always good things to do before submitting a pull request.

As for the changes, 👍. I bet yours is not the only IDE that has trouble with this. Thanks for contributing! I hope the failing build didn't scare you off from future contributions to request :).

@simov I don't think we should not make code changes just to not please someone's editor. There were >9M downloads last month, so we should try to make it a pleasure to develop with request for as many users as possible, whenever possible.

[froatsnook]

@flanneljesus Feel free to squash together your two commits into one. That way there's less noise in the diffs, in the git blame, etc. Since no one else is tracking the flannelJesus/fix-verbs branch, it's not a faux-pas to rewrite history. In case you've never done this before, you can:

Do an interactive rebase onto two commits before HEAD:

git rebase -i HEAD~~

That opens your editor with this:

pick f5eaea8 fix the way http verbs are defined
pick a63a3fc fix formatting according to eslint settings

Then change the second pick to fixup like this:

pick f5eaea8 fix the way http verbs are defined
fixup a63a3fc fix formatting according to eslint settings

and save and quit. git will merge together the two commits into one and take the commit message from the first. Interactive rebase is super powerful. Instead of fixup, you can use squash (if you want to edit the commit message of the combined commit) or reword if you just want to change the commit message. You can also reorder commits (just by reordering the lines in this file) and squash together multiple commits (put multiple fixup/squash in a row). One thing I often do is find a typo from a few commits ago. In order to fix that, you add a new commit that fixes it, then do an interactive rebase and reorder the lines and change the new commit to fixup. It's like the typo never happened (again unless someone is tracking your branch, in which case rewriting history is ugly). There might be merge conflicts when you reorder commits, so be careful with that.

After you change your history, you have to do a forced push to github:

git push --force origin fix-verbs

This automatically updates the pull request!

[simov]

@froatsnook I'm not totally against that change as it's not that huge (for such issue). My point was that we already moved from request.method to the array approach in #1435 and since there is no active constraint that we can enable regarding that behavior, future changes might brake it again.

The only constraint that I can think of is someone using WebStorm, or anyone else relying on codeintel plugin in their IDE. Anyway, we're not loosing that much in terms of DRYing up those methods, so I'll consider this PR as an actual fix.

[samjross]

Thank you froatsnook, I've done the rebase as you suggested.

And thanks both of you for being patient with a...newb... I'm working on it 😉

[froatsnook]

@simov I definitely agree that the code shouldn't get worse to make editors happy.

@flanneljesus I'm glad the rebase worked. It always makes me a bit nervous :).

[simov]

Merged with one small final touch from me 👍