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.

Sign up for GitHub

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

Jump to bottom

Test + fix for get-commit-test.js to return correct path in branchUrl #192

Merged
merged 8 commits into from
May 1, 2018
Merged

Test + fix for get-commit-test.js to return correct path in branchUrl #192

merged 8 commits into from
May 1, 2018

Conversation

jywarren
Copy link
Contributor

@jywarren jywarren commented Feb 26, 2018 •
edited
Loading

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 of blob/branch-name/path/to/file.rb and replacing, instead of just blob/branch-name.

@jywarren jywarren mentioned this pull request Feb 26, 2018
Closed
@jywarren jywarren closed this Feb 26, 2018
@jywarren jywarren reopened this Feb 26, 2018
@jywarren jywarren changed the title Attempt to get get-commit-test.js to return bad branchUrl Test + fix for get-commit-test.js to return correct path in branchUrl Feb 26, 2018
@jywarren
Copy link
Contributor Author

@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!

This was referenced Apr 26, 2018
Closed
Open
@jywarren
Copy link
Contributor Author

Hi, @gr2m -- will this work for you? Thanks!

@jywarren
Copy link
Contributor Author

Hmm, additional commits pushed this out of sync...

@jywarren
Copy link
Contributor Author

It's weird, I remove docs from this line and then it switches to requiring docs... back and forth?

test/unit/get-commit-test.js .......................... 8/9
  get commit request succeeds
  not ok should be equal
    +++ found
    --- wanted
    -https://github.com/Techforchange/first-timers-test/blob/defaultBranch/docs/README.md
    +https://github.com/Techforchange/first-timers-test/blob/defaultBranch/README.md

@jywarren
Copy link
Contributor Author

Any help appreciated! This is a small but important barrier we're trying to fix for our newcomers. Thank you!!! 😄

@jywarren
Copy link
Contributor Author

As of 14b9efc it definitely worked!

@gr2m
Copy link
Contributor

gr2m commented Apr 28, 2018

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

gr2m
gr2m reviewed Apr 28, 2018
View reviewed changes
test/unit/get-commit-test.js
@@ -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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jywarren
Copy link
Contributor Author

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?

@jywarren
Copy link
Contributor Author

OK, phew! This looks good again now. Will this work for squash-and-merge? Thanks @gr2m !!

@gr2m
Copy link
Contributor

gr2m commented May 1, 2018

Sorry for the extra commits but it'll all be squashed anyways, right?

Yes no problem at all :)

gr2m
gr2m approved these changes May 1, 2018
View reviewed changes
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 thank you!

@gr2m gr2m merged commit 1f53046 into first-timers:master May 1, 2018
@jywarren
Copy link
Contributor Author

jywarren commented May 1, 2018 via email

@gr2m
Copy link
Contributor

gr2m commented May 1, 2018

Thanks for the kind words :)

Do I need to take additional steps to see this deployed on our
install of first-timers-bot?

The app is deployed automatically whenever we merge something to master, so it should be up already :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gr2m gr2m gr2m approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jywarren @gr2m