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

giturlparse vs git-url-parse #277

Closed
jayvdb opened this issue Jul 8, 2019 · 4 comments · Fixed by #288
Closed

giturlparse vs git-url-parse #277

jayvdb opened this issue Jul 8, 2019 · 4 comments · Fixed by #288
Assignees

Comments

@jayvdb
Copy link
Member

jayvdb commented Jul 8, 2019

#227 switched from git-url-parse to giturlparse , due to #226 . Originally added in #207

(There is also a giturlparse.py , the predecessor of giturlparse)

Both install into site-packages giturlparse, so it is impossible to have both installed as dependencies.

coala uses git-url-parse, but giturlparse is starting to get maintained.

python-gilt switched to git-url-parse retr0h/py-gilt@673093d
lancet switched GaretJax/lancet@7ba7d2e#diff-b4ef698db8ca845e5845c4618278f29aL17

As a result it is impossible to install moban in the same environment in the same environment as coala, gilt or lancet.

openSUSE has git-url-parse already

However arch has https://aur.archlinux.org/packages/python-giturlparse/

Most distros have neither.

@jayvdb
Copy link
Member Author

jayvdb commented Jul 8, 2019

fwiw, if there are needed features in https://github.com/nephila/giturlparse , not present in https://github.com/retr0h/git-url-parse , coala could switch.

@jayvdb jayvdb self-assigned this Jul 8, 2019
@jayvdb
Copy link
Member Author

jayvdb commented Jul 8, 2019

coala could switch.

Actually, I suspect this is not true. retr0h/git-url-parse parses URLs without both user/repo which is needed in some parts of coala (the community GCI app?). nephila/giturlparse rejects them as invalid.

The problem of #226 is that git-url-parse had some bugs. I suspect https://github.com/retr0h/git-url-parse/issues/29 was raised about one of them. That bug was fixed, but the issue has been disappeared by @retr0h. I see it still doesnt support trailing slashes, which is also another problem with the same scenario.

It has not tests for trailing slashes either. https://travis-ci.org/jayvdb/git-url-parse/builds/555748106 It has 26 parsing test cases.

However, giturlparse also doesnt support trailing slashes; except it does return something which can be manipulated to be correct.

And it also had no tests for trailing slashes. https://travis-ci.org/jayvdb/giturlparse/builds/555755629 . It has 11 parsing test cases.

There are lots of cases from git-url-parse which fail on giturlparse. Some are intentional failures.

OTOH, wrt https://github.com/retr0h/git-url-parse , @retr0h has disabled the GitHub issues feature. That is a major problem, especially as the issues are mentioned in the source code, so we need to guess what issues were created by whom, and what they said.

There were also a few nephila/giturlparse test cases which failed on retr0h/git-url-parse, so I have created two PRs. https://github.com/retr0h/git-url-parse/pull/38 and https://github.com/retr0h/git-url-parse/pull/39

@jayvdb
Copy link
Member Author

jayvdb commented Jul 9, 2019

moremoban/pypi-mobans@eedde0f4e also refers to https://github.com/retr0h/git-url-parse/issues/29 , so I am fairly confident it was created by someone from @coala / @moremoban

jayvdb added a commit to jayvdb/moban that referenced this issue Jul 9, 2019
jayvdb added a commit to jayvdb/moban that referenced this issue Jul 9, 2019
This substantially reverts commit 9517891.

The cause of the change has been addressed by an enhancement in
git-url-parse v1.2.1, too late to be included in moban v0.4.0
retr0h/git-url-parse@3e2bf91e

Using giturlparse instead of git-url-parse is a non-trivial breaking change.
It was done to avoid one minor regression which users could workaround,
however it introduced many significant regressions users could not
workaround as giturlparse does not parse many types of URLs.

Closes moremoban#277
jayvdb added a commit to jayvdb/moban that referenced this issue Jul 9, 2019
git-url-parse does not yet (1.2.2) handle trailing slashes,
which are an optional part of all git URLs, but were supported
before the switch to using GitPython.

Closes moremoban#277
jayvdb added a commit to jayvdb/moban that referenced this issue Jul 9, 2019
This substantially reverts commit 9517891.

The cause of the change has been addressed by an enhancement in
git-url-parse v1.2.1, too late to be included in moban v0.4.0
retr0h/git-url-parse@3e2bf91e

Using giturlparse instead of git-url-parse is a non-trivial breaking change.
It was done to avoid one minor regression which users could workaround,
however it introduced many significant regressions users could not
workaround as giturlparse does not parse many types of URLs.

Closes moremoban#277
jayvdb added a commit to jayvdb/moban that referenced this issue Jul 9, 2019
git-url-parse does not yet (1.2.2) handle trailing slashes,
which are an optional part of all git URLs, but were supported
before the switch to using GitPython.

Closes moremoban#277
jayvdb added a commit to jayvdb/moban that referenced this issue Jul 9, 2019
This substantially reverts commit 9517891.

The cause of the change has been addressed by an enhancement in
git-url-parse v1.2.1, too late to be included in moban v0.4.0
retr0h/git-url-parse@3e2bf91e

Using giturlparse instead of git-url-parse is a non-trivial breaking change.
It was done to avoid one minor regression which users could workaround,
however it introduced many significant regressions users could not
workaround as giturlparse does not parse many types of URLs.

Closes moremoban#277
jayvdb added a commit to jayvdb/moban that referenced this issue Jul 9, 2019
git-url-parse does not yet (1.2.2) handle trailing slashes,
which are an optional part of all git URLs, but were supported
before the switch to using GitPython.

Closes moremoban#277
@retr0h
Copy link

retr0h commented Jul 9, 2019

Send me PRs to @git-url-parse and I'll merge them.

@chfw chfw closed this as completed in #288 Jul 10, 2019
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 a pull request may close this issue.

2 participants