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

fix the way http verbs are defined #1598

Merged
merged 1 commit into from
May 25, 2015
Merged

Conversation

samjross
Copy link
Contributor

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
Copy link
Contributor Author

looks like I messed up on formatting. sorry.

@simov
Copy link
Member

simov commented May 24, 2015

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 samjross closed this May 24, 2015
@samjross samjross reopened this May 24, 2015
@samjross
Copy link
Contributor Author

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

@froatsnook
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Member

simov commented May 24, 2015

@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.

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
Copy link
Contributor Author

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
Copy link
Contributor

@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 simov merged commit 9c9918b into request:master May 25, 2015
simov added a commit that referenced this pull request May 25, 2015
Fix the way http verbs are defined in order to please intellisense IDEs
@simov
Copy link
Member

simov commented May 25, 2015

Merged with one small final touch from me 👍

@samjross samjross deleted the fix-verbs branch May 25, 2015 18:31
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