Skip to content
This repository has been archived by the owner on Oct 24, 2019. It is now read-only.

Fix #70 #72

Closed
wants to merge 1 commit into from
Closed

Fix #70 #72

wants to merge 1 commit into from

Conversation

mdsb100
Copy link
Contributor

@mdsb100 mdsb100 commented Feb 12, 2015

No description provided.

@dave-irvine
Copy link
Contributor

Ok I need @moul to weigh in on this, but if we are passing in a params argument, shouldn't projectId and hookId be contained within params now?

@mdsb100
Copy link
Contributor Author

mdsb100 commented Feb 12, 2015

add: (projectId, params, fn = null) =>
    if 'string' is typeof params
      params =
        url: params
    @debug "Projects::addHook()"
    @post "projects/#{Utils.parseProjectId projectId}/hooks", params, (data) => fn data if fn

I just imitate the function "add" in ProjectHooks.coffee.
I read api/projects, I prefer this:

update: (projectId, hookId, url, params = {}, fn = null) =>
    @debug "Projects::saveHook()"
    params.url = url
    @put "projects/#{Utils.parseProjectId projectId}/hooks/#{parseInt hookId}", params, (data) => fn data if fn

@moul
Copy link
Owner

moul commented Feb 12, 2015

It sounds like #33
I think too that a common interface everywhere would be the best solution, but it will also break the whole compatibility and need a new major release

since we don't have named arguments in javascript, I think the object params is the best solution to handle optional parameters and to help people call functions without thinking about parameters order

We need more thought, but I personally like the idea of standardised parameters everywhere, it will also be easier to use a marshaller to validate arguments

@moul moul added the question label Feb 12, 2015
@dave-irvine dave-irvine mentioned this pull request Mar 12, 2015
5 tasks
@moul moul closed this Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants