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.

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

Attempt to correct linked file regEx in branchUrl #190

Closed
wants to merge 1 commit into from

Conversation

jywarren
Copy link
Contributor

Hi, I /think/ this is correct:

(.+) 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.

...but am not sure of branch name conventions. Will (\w+) correctly grab any branch name?

@gr2m
Copy link
Contributor

gr2m commented Feb 26, 2018

I fear that’s not enough, branch names can include - and even / ... maybe we can get the branch name entirely from a different property? 🤔

@gr2m
Copy link
Contributor

gr2m commented Feb 26, 2018

a wait, no we are fine, because the blob_url contains a commit sha in the response, not a branch name, so yeah, we should be fine.

Could you try to add a test covering the change, so we don’t break it in future?

@jywarren
Copy link
Contributor Author

Whoa, but does GitHub generate URLs that include / in branch names? Or does it convert them? Is the limitation GitHub or Git?

We could also compare it to the filename, so basically snip out the filename then subtract the relative path from the blob_url to get the branchUrl?

@jywarren
Copy link
Contributor Author

commit sha - yes, that's what I thought. cool!

@jywarren
Copy link
Contributor Author

Let me look at the tests, hang on a sec!

jywarren added a commit to jywarren/first-timers-bot that referenced this pull request Feb 26, 2018
@jywarren
Copy link
Contributor Author

Sorry, i'm making a real mess of my PRs.

I'm trying to open a PR with just the test to let it fail first.

@jywarren
Copy link
Contributor Author

OK, so for what it's worth, in my PR mess, i now have all three states :-/ -- well, don't we all love empiricism:

  1. this PR has the fix but not the test
  2. Test + fix for get-commit-test.js to return correct path in branchUrl #192 demonstrates the test (failing) without the fix
  3. Attempt to fail test due to #189 #191 demonstrates both the test and the fix (passing)

We can move to #191 or I can merge it into here too?

@gr2m
Copy link
Contributor

gr2m commented Feb 26, 2018

Ideally, to make my life easiest :D you do the following

  1. close all open PRs
  2. Create a new PR with one commit that only has the test, it will show the error status on that commit.
  3. Then push the fix, which will change it to success

see https://twitter.com/gr2m/status/963879699385495552 :)

@jywarren
Copy link
Contributor Author

OK, I'm closing this, and moving to #192 which demonstrates the test failing. I'll add the fix and then will see the green ✔️ get added there. Thanks @gr2m !

@jywarren jywarren closed this Feb 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants