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

only replace last @ after (if any) last / with # #2395

Merged
merged 2 commits into from Mar 28, 2018

Conversation

doomedramen
Copy link
Member

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

@ankon
Copy link

ankon commented Nov 22, 2016

Some notes:

  • The diff contains a lot of whitespace changes, which makes reading a bit harder.
  • In both cases the fix happens just before an "endpoint parser" is invoked - wouldn't it make sense to move that fix into that one instead? The code duplication now screams for getting missed the next time the url parser has to be touched.
  • Would it make sense to use an existing class to parse URI/URLs, using regular expressions and string.split tends to break quickly usually :)

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

@doomedramen
Copy link
Member Author

@ankon thanks for looking at this.

  • I was told that if my editor fixes any code formatting I should commit it.
  • I was asked not to modify the endpoint parser, I avoid regex where possible because if someone does not know it well its hard to debug but standard js is understandable.
  • Good point.

@ankon
Copy link

ankon commented Dec 6, 2016

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.

@doomedramen
Copy link
Member Author

I dont know, it is waiting on someone like @sheerun to accept it.
It could do with more in depth testing but would required testing of git@github.com/user/repo.git urls (which I cant find in the test code).

admwx7
admwx7 approved these changes Dec 13, 2016
Copy link

@admwx7 admwx7 left a 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!

@admwx7
Copy link

admwx7 commented Dec 13, 2016

I know you mentioned you don't like regex @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 #

@doomedramen
Copy link
Member Author

doomedramen commented Dec 13, 2016 via email

@admwx7
Copy link

admwx7 commented Dec 13, 2016

True, I'm just worried about the case where a user is installing via a branch name that contains / in it:

git@github.com/user/repo.git@feature/new-feature

Since it's running off last segment split on / it would find there is no @ and fail later on.

@robertmassaioli
Copy link

Ah, I think that I just ran into this:

> bower "install" "purescript-setdown"

bower purescript-setdown#*  not-cached git@bitbucket.org:robertmassaioli/purescript-setdown.git#*
bower purescript-setdown#*     resolve git@bitbucket.org:robertmassaioli/purescript-setdown.git#*
bower purescript-setdown#*    checkout master
bower purescript-setdown#*    progress remote: Compressing objects:  86% (124/144)
bower purescript-setdown#*    progress remote: Compressing objects:  87% (126/144)
bower purescript-setdown#*    resolved git@bitbucket.org:robertmassaioli/purescript-setdown.git#1ffa0bb8bd
bower                        ENOTFOUND Package purescript-setdown not found

@Masadow
Copy link

Masadow commented Feb 10, 2017

There's also this syntax for private repositories : git+https://<TOKEN>:x-oauth-basic@github.com/org/repo#branch

@jameelmoses
Copy link

Any word on this? Still stuck at 1.7.9 until there's a fix

@doomedramen
Copy link
Member Author

If anyone has a better solution than the one I have proposed please put in a PR and link to here. Thank you

@mryellow
Copy link

If anyone has a better solution

Think unbreaking the release should be priority.

Really couldn't care less about whitespace changes when builds stop working.

@nikhil-batra
Copy link

Hello, Is there any update regarding merge of this pull request?

@HansWouters
Copy link

If you're looking to not use split, might this be a solution?
endpoint.replace(/(\/.*)@([^\/]*)$/, '$1#$2');
Replaces only the @ after a / and if no / follows.

See https://jsbin.com/cabaxofejo/edit?js,console

@serenanubi
Copy link

Bump?

@stramel
Copy link

stramel commented Oct 17, 2017

This is still an issue with version 1.8.2 from the CLI. It seems like running bower install with the URL in the bower.json works but the CLI doesn't resolve it properly yet.

@bennypowers
Copy link

@stramel I have this issue on CLI 1.8.2, but not as you suggested when running bower i on a fully populated bower.json

Copy link

@mikef438 mikef438 left a 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.

@mryellow
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet