- Notifications
You must be signed in to change notification settings - Fork786
fix(repository): prevents lib from crashing when not providing option…#588
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
test/repository.spec.js Outdated
| }); | ||
| it('should successfully write to repo when not providing optional options argument',function(done){ | ||
| remoteRepo.writeFile('master',fileName,initialText,initialMessage,undefined,assertSuccessful(done,function(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the reason people are running into this bug is because they're using Promises. The 2 option-less calls I'm imagining people are making:
- Using callbacks:
remoteRepo.writeFile('master',fileName,initialText,initialMessage,assertSuccessFunction)
- Using Promises:
remoteRepo.writeFile('master',fileName,initialText,initialMessage).then(res=>{})
I'd like tests to verify the Promise approach, as we have many testing writing files with no options provided, but with a callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
True. Thinking back, I stumbled upon this using async/await. If one were to use the callback approach this would work fine since there is a check in the beginning to see ifoptions is a function and in that case use that as the callback.
j-rewerts commentedOct 8, 2019
Apologies for the delay@hazmah0. I've been traveling. Please make the requested changes. As for the testing account, feel free to email me and I can get you access, or alternatively, feel free to use one of your own GitHub accounts. Just note that running the test suite will create and edit a bunch of things, so it's best to avoid using your primary account. |
j-rewerts commentedOct 8, 2019
Also, I'm not too sure how Hacktoberfest counts contributions, but if you need to close and reopen this to get it to count towards your 5, feel free to do that. |
hazmah0 commentedOct 10, 2019
No worries about the contribution count. I'll send an email later today, thanks! |
j-rewerts commentedOct 30, 2019
LGTM! |
hazmah0 commentedNov 4, 2019
Just noticed that the CI is failing the lint task due to a missing semicolon at line 395. I can submit a new PR with that fix later today! |
…al arguments
Ran into this issue today described in this old PR:#456
This provides the same fix and adds a test case, though I can't run the tests since it asks me to emailjaredrewerts@gmail.com to get access.