Test + fix for get-commit-test.js to return correct path in branchUrl #192
Conversation
jywarren
changed the title
|
@gr2m - here's the PR with both the failing test and the commit that fixes it, both with CI test results showing. Does this work for you? Thanks! |
|
Hi, @gr2m -- will this work for you? Thanks! |
|
Hmm, additional commits pushed this out of sync... |
|
It's weird, I remove |
|
Any help appreciated! This is a small but important barrier we're trying to fix for our newcomers. Thank you!!! 😄 |
|
As of 14b9efc it definitely worked! |
|
hey @jywarren I’m so sorry to not get back to you sooner, the PR and issue somehow managed to slipped out of my todo list! I’m looking into it now |
| @@ -47,7 +47,7 @@ test('get commit request succeeds', t => { | |||
| t.is(state.commit.message, 'message') | |||
| t.is(state.commit.filename, 'filename') | |||
| t.is(state.commit.patch, 'patch') | |||
| t.is(state.commit.branchUrl, 'https://github.com/Techforchange/first-timers-test/blob/defaultBranch/README.md') | |||
| t.is(state.commit.branchUrl, 'https://github.com/Techforchange/first-timers-test/blob/defaultBranch/docs/README.md') | |||
There was a problem hiding this comment.
you have to change line 28 to make it work
- blob_url: 'https://github.com/Techforchange/first-timers-test/blob/f00ecb1bbffe515500558568ae0b176d2a1defe8/README.md'
+ blob_url: 'https://github.com/Techforchange/first-timers-test/blob/f00ecb1bbffe515500558568ae0b176d2a1defe8/docs/README.md'
lib/get-commit.js
Outdated
| @@ -11,7 +11,7 @@ function getCommit (state) { | |||
|
|
|||
| .then(function (result) { | |||
| const {filename, patch, blob_url: blobUrl} = result.data.files[0] | |||
| var branchUrl = blobUrl.replace(/(?:blob)(.+)(?=\/)/g, 'blob/' + state.repoDefaultBranch) | |||
| var branchUrl = blobUrl.replace(/(?:blob\/)(\w+)/g, 'blob/' + state.repoDefaultBranch) | |||
There was a problem hiding this comment.
I think we I’d change the regex pattern
- /(?:blob\/)(\w+)/g
+ /\/blob\/[^/]+/That should make it more clear. We don’t need the g modifier, either, it’s a single replace.
The regex will cause problems in edge cases where a repository owner or repository name is "blob", but let’s create a follow up issue for that, it’s out of scope of this PR
|
Ugh, that was an oversight! Thanks for the catch, let's see how this does. Sorry for the extra commits but it'll all be squashed anyways, right? |
|
OK, phew! This looks good again now. Will this work for squash-and-merge? Thanks @gr2m !! |
Yes no problem at all :) |
|
Awesome! Do I need to take additional steps to see this deployed on our
install of first-timers-bot? Thanks so much!!!! That bot has been a really
awesome tool for us. Very grateful.
…On Mon, Apr 30, 2018 at 8:50 PM, Gregor Martynus ***@***.***> wrote:
Merged #192 <#192>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#192 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ07lZxGrIEWuY9Ro0EnC3QBPhbsLks5tt7FRgaJpZM4ST8Ji>
.
|
|
Thanks for the kind words :)
The app is deployed automatically whenever we merge something to master, so it should be up already :) |
This test highlights an issue which I have written a fix for; adding the fix as a next step. Thanks!
(.+)is changed to(\w+)and the slash is moved inside the "blob" bit. I tested and it correctly addresses the issue in #189. It had been grabbing all ofblob/branch-name/path/to/file.rband replacing, instead of justblob/branch-name.