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

Legacy documentation #3

Closed
wants to merge 20 commits into from
Closed

Legacy documentation #3

wants to merge 20 commits into from

Conversation

demurgos
Copy link
Member

Hi!
This pull request is a follow up of this issue.

I tried to document most of the current/legacy (2.x.x) version of request in order to add missing methods/properties, clearly define the types & default values, split the huge README.md in smaller chunks, add prototype chains when needed and add installation/usage sections. I also tried to add links to external documentation when some comments were referring to it (mostly to the Node and tough-cookie API).

I still left many TODOs: the Request class (return value for the main functions) lacks documentation (ie. what should be public ?) and the request-options.md file also needs some precisions.

My PR might not be complete, but I hope that we will be a starting point to fix the missing parts and finally start using this website. As mentioned in the previous issue, my goal is also to provide better support for the Typescript typings so I would like some returns about my documentation of the public API.

@simov
Copy link
Member

simov commented Aug 28, 2016

Hey @demurgos I just invited you to join Website team. Check out the contributing guideline on what you can do next. Good job 👍

@simov
Copy link
Member

simov commented Aug 28, 2016

Also feel free to update the authentication and cookies docs, the files that are currently there are just placeholders.

@demurgos
Copy link
Member Author

Thank you!
I'll try to advance on this this week.

@demurgos
Copy link
Member Author

demurgos commented Sep 21, 2016

Hi,
This is just an update about my progression on the legacy API. The structure has not changed that much, I just separated the API of the main module and the index/overview (main module, options, classes, etc.) to two different files. The main module API is a bit terse but complete.

I finally started to review the options' documentation but its pretty slow. I add examples and try to clarify dependencies between options (the querystring part was pretty confusing). I also add many TODOs when there are some points that require some checking so I would appreciate some help to solve these. (For example: Are custom HTTP methods handled ? What is the interaction between request.defaults and options such as header or qs ?)

I keep advancing when I have some free time but if someone wants to help I'd be happy to share some of the work :D.

@simov
Copy link
Member

simov commented Sep 21, 2016

Request supports both the qs module and the default querystring module from Node Core. That's why there are two separate set of options for each module respectively.

Options defined in defaults get merged in (deep) with the rest of the options before making the actual request.

You can specify any HTTP verb as far as I know.

Probably a better approach for such huge and ongoing task would be to create separate branch here. That way it will be easier for us to contribute. Lastly you need to create PR from that branch to master.

@demurgos
Copy link
Member Author

Thank you for your answers.

I understood how query strings work thanks to .useQuerystring, but if you only read the other related
options it wasn't clear. Thank you for confirming that you can switch between libraries.

About the merge, objects are deeply merged but what about arrays ? Are they replaced, concatenated or some trickier transformation ?

I do not have the permission to create a separate branch, could you do it ?

@simov
Copy link
Member

simov commented Sep 22, 2016

Arrays are deep merged too.

You should be able to create a new branch, though I just created legacy-doc out from master. Here is what you should do:

  1. Pull legacy-doc from this repo
  2. Add all of your work there
  3. Push it to legacy-doc here
  4. Create pull request, just make sure you set the branches correctly - you want to merge legacy-doc to master

@demurgos
Copy link
Member Author

demurgos commented Sep 24, 2016

3 . Push it to legacy-doc here

I got a 403 (unauthorized) error when pushing to this repo. Am I supposed to have write access ? I can see my account in the members section but I have to admit that I don't exactly what it implies, does it affects my permissions ?

@simov
Copy link
Member

simov commented Sep 24, 2016

Well, your invitation is still pending. You have to click on the link in the email that GitHub sent you:

Invitations are sent via email and can be accepted at https://github.com/request

@demurgos
Copy link
Member Author

Ok, thank you. It was my first invitation so I was looking for a message in the "notifications" page.

I just pushed my changes to legacy-doc, I'll create the new PR later today.

What is request's workflow to push changes when you have write access ?

  • Push changes to your personal fork and then do a PR from your repo to this repo (as when you have no write access).
  • Push each change on a new branch in this repo and then do a PR from this feature branch to the master branch (or a secondary branch like legacy-doc) ?

Basically, should I work on new branches in this repo or in my fork ?

@demurgos demurgos closed this Sep 25, 2016
@simov
Copy link
Member

simov commented Sep 25, 2016

I don't think we have strict policy on that. My thinking is that if it is something small you can quickly push to your own fork and open PR. If you expect others to contribute to your work before merging it in then create a branch. BTW recently GitHub introduced new feature but I haven't tested it yet. When you are creating a PR from your fork there is a small checkbox down at the bottom indicating whether you allow contributors to the upstream repo to push to your own fork.

@demurgos
Copy link
Member Author

Right, I see this checkbox ("Allow edits from maintainers"). It's good to know.

If you expect others to contribute to your work before merging it in then create a branch.

This seems like a good test. Thank you for your comment.

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.

None yet

2 participants