search

github-tools / github

A higher-level wrapper around the Github API. Intended for the browser.
BSD 3-Clause "New" or "Revised" License
3.6k stars 755 forks source link

Repository.writeFile fails if no options object is passed #577

Open lantchou opened 5 years ago

lantchou commented 5 years ago

In the documentation about Repository.writeFile, the options parameter is specified as optional. Despite this, the function fails if no options object is passed and throws this error:

TypeError: Cannot read property 'encode' of undefined
    at Repository.writeFile (/Users/lucasantchougov/Documents/Work/oauth-rule-manager/node_modules/github-api/dist/components/Repository.js:941:37)
    at GitHubRuleManager.commitWorkspace (/Users/lucasantchougov/Documents/Work/oauth-rule-manager/lib/rule_managers/github-rule-manager.js:143:18)
    at /Users/lucasantchougov/Documents/Work/matey/matey-server/app.js:74:25
    at Layer.handle [as handle_request] (/Users/lucasantchougov/Documents/Work/matey/matey-server/node_modules/express/lib/router/layer.js:95:5)
    at next (/Users/lucasantchougov/Documents/Work/matey/matey-server/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/Users/lucasantchougov/Documents/Work/matey/matey-server/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/Users/lucasantchougov/Documents/Work/matey/matey-server/node_modules/express/lib/router/layer.js:95:5)
    at /Users/lucasantchougov/Documents/Work/matey/matey-server/node_modules/express/lib/router/index.js:281:22
    at param (/Users/lucasantchougov/Documents/Work/matey/matey-server/node_modules/express/lib/router/index.js:354:14)
    at param (/Users/lucasantchougov/Documents/Work/matey/matey-server/node_modules/express/lib/router/index.js:365:14)

The error seems to get thrown when options.encode is read in Repository.writeFile. It only works for me when passing an empty object. This should be better documented or a default empty object should be instantiated in the method.

CodyGramlich commented 4 years ago

It seems to work without having the options parameter when the callback function is used, but ideally you should be able to call writeFile without either the options parameter or the callback function. I will look into this.

CodyGramlich commented 4 years ago

I just realized this is solved in PR #588.

j-rewerts commented 4 years ago

After looking at this a little more closely, it looks like our old approach was to require a callback. If a function was provided as the options param, we would assume that the user was passing in the callback function. When we added promises, people stopped using callbacks, so we can no longer assume that this is a required parameter (even though the docs say it is).