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 documentation of parameters #110

Open
Fishbowler opened this issue Jan 30, 2018 · 4 comments
Open

Fix documentation of parameters #110

Fishbowler opened this issue Jan 30, 2018 · 4 comments

Comments

@Fishbowler
Copy link
Collaborator

Source

The last round of documentation added some reverse engineered params that are explicitly used.
In fact, we've got some of our own, plus everything that request supports.

@Fishbowler
Copy link
Collaborator Author

Once #106 is merged, we're closer.

config(opts) takes an object:

{
  "inspectOnFailure": true,
  "json": true,
  "timeout": 2000,
  "retry": 0,
  "request": {
    "proxy": "http://proxy.domain.net:8080",
    "followRedirect": false,
    "form": false
  }
}

Here, anything in request is for the request module. Our json value later gets merged into request's value. Same for timeout, etc.

post(uri, body, params) has a params object that is a mix of the above. It's the request section above, where we use the json value, but so does request, ditto timeout, and we also see our own parameter of mock added, that never gets given over to request since it isn't used when mock is present. inspectOnFailure is never set by this method.

"request": {
  "proxy": "http://proxy.domain.net:8080",
  "followRedirect": false,
  "json": true,
  "timeout": 2000
  "mock": {...}
}

To simplify the docs, we should standardise on one method or another. I like that we have our options for our use that we then pass to request, but also like the simplicity of using just the request block. I could be easily led by either.

In order to standardise, I'd like to remove params from method calls as part of v2 now that we've got config in #106. If I can get some opinions on direction, I'll happily start this work right now.

@paulmelnikow
Copy link
Collaborator

mock is deprecated; it depends on mock-request, which is an obsolete mock library. We could mark it deprecated now and remove it once we get our tests moved over to nock. Does that make sense, and seem consistent with your usage?

To simplify the docs, we should standardise on one method or another. I like that we have our options for our use that we then pass to request, but also like the simplicity of using just the request block. I could be easily led by either.

In order to standardise, I'd like to remove params from method calls as part of v2 now that we've got config in #106.

Could I ask you to clarify this? A little time has passed. It sounds like you're leaning toward dropping params from all the method calls. Is that right?

By the way, I want to apologize! I didn't had time to work on open source the last month or so. Lots of stuff in home and work life has been busy! In the future feel free to hit me up on email or twitter if you're not getting a response here.

@Fishbowler
Copy link
Collaborator Author

Absolutely that. With globalConfig gone, we've now got 2 methods of doing the same thing "in line" with a request. Removing the params simplifies the framework whilst also solving #112.

@paulmelnikow
Copy link
Collaborator

Great. Sounds good to me. If removing it creates duplication and developers want to avoid duplicate setup() calls, they can write a function to wrap the create() and setup calls().

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

No branches or pull requests

2 participants