-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
only replace last @
after (if any) last /
with #
#2395
Conversation
Some notes:
(FWIW: I have not looked at the bigger bower code base, so these points are just things I noticed when trying to understand the fix :D) |
@ankon thanks for looking at this.
|
Is there anything I can help with to get this merged quicker? My builds are failing due to this, and I can go either forward to a fixed version, or force a downgrade everywhere that I would need to undo then when the issue has been resolved. |
I dont know, it is waiting on someone like @sheerun to accept it. |
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.
LGTM, thanks for taking the time!
I know you mentioned you don't like regex @wookoouk but you could clean things up a bit using a very simple one:
And it's a bit more descriptive since according to the API docs the only currently supported usage of |
People are not just using git@, they are also doing USERNAME@
…On Tue, 13 Dec 2016, 18:36 Austin Murdock, ***@***.***> wrote:
I know you mentioned you don't like regex @wookoouk
<https://github.com/wookoouk> but you could clean things up a bit using a
very simple one:
endpoint.replace(/([^git])@/, function(match, prefix) {
return prefix + '#';
});
And it's a bit more descriptive since according to the API docs the only
currently supported usage of @ is git@, so it's fair to assume all other
usages should be converted to #
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2395 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAbODLDjyWBIyBVpdWA-K6jqq5NEur_yks5rHuXGgaJpZM4K5t2F>
.
|
True, I'm just worried about the case where a user is installing via a branch name that contains
Since it's running off last segment split on |
Ah, I think that I just ran into this:
|
There's also this syntax for private repositories : |
Any word on this? Still stuck at 1.7.9 until there's a fix |
If anyone has a better solution than the one I have proposed please put in a PR and link to here. Thank you |
Think unbreaking the release should be priority. Really couldn't care less about whitespace changes when builds stop working. |
Hello, Is there any update regarding merge of this pull request? |
If you're looking to not use split, might this be a solution? |
Bump? |
This is still an issue with version |
@stramel I have this issue on CLI 1.8.2, but not as you suggested when running |
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 tested this locally and it fixes the issue with installing from a git repo with "@" in the URL.
@mikef438 The hero we need. Can't believe a critical issue which broke builds across the globe took so long to merge simply because someone didn't like the whitespace. |
I was previously unaware that some of these (https://bower.io/docs/api/#install) url styles were allowed, I have changed the code to check if there are any
/
and make sure to only replace@
symbols with#
after the last one