Stricter repo parsing #19
Conversation
* Domain must not include a slash * Org must not include a slash on github
giturlparse/tests/parse.py
Outdated
| 'host': 'gitlab.com', | ||
| 'user': 'git', | ||
| 'owner': 'nephila/giturlparse/blob/master/giturlparse', | ||
| 'repo': 'github.py', |
There was a problem hiding this comment.
If I understand correctly, previously github.py would end up in the host or something like that.
So while this is a bad result for repo on GitLab, it is less bad than before this PR, and at least it is working much better for github.
IMO, "owner" shouldnt contain a / on GitLab either. Subgroups should be accessed using a new member named something like path / pathname c.f. https://github.com/retr0h/git-url-parse/pull/37 Or departing from the urlparse tuple, a member called groups which is a list of strings.
There was a problem hiding this comment.
@jayvdb this is as far as I am comfortable going with supporting the different system details
what would you think?
There was a problem hiding this comment.
Up to you. fwiw, https://github.com/coala/git-url-parse has just been transferred to the @coala group, which means we can put our orgs needs into that codebase. Which then leads to how do we cooperate/compete for the name? c.f. coala/git-url-parse#43
There was a problem hiding this comment.
Sorry if the wording sounded a bit rude, it was not meant. It's just that regexps are a PITA to maintain beyond a certain complexity :)
I also forgot to mention that I updated the PR based on your suggestions, and this PR should be able to support more gitlab features in a more natural way.
Regarding git-url-parse, it certainly isn't super smart to have two competing and very similar implementation that covers this use case, thus definitely let's talk about this. I will answer in the linked issue to stay on topic
yakky
force-pushed
the
feature/strict-regexp
branch
from
6f7a2d7 to
161bdf2
Compare
giturlparse/result.py
Outdated
| @property | ||
| def path(self): | ||
| if self.raw_path: | ||
| return self.raw_path.replace('/blob/', '').strip('/') |
There was a problem hiding this comment.
I dont see the benefit of removing only blob (and not other REST 'commands' like 'tree'), but not also remove the branch name. Keeping the branch name in the path makes it like SVN, but for most VCS the branch and the rest of the path are separate and are used in different commands/parts of the workflow. If they are combined here, people will usually need to split it before they can use it.
If the branch name is also removed, path becomes synonymous with the file path that is found in a local clone.
Exposing the branch as a separate attribute would be useful, but can be done later.
fwiw, git-url-parse uses pathname for what you have exposed as raw_path, to correspond with urlparse()'s pathname. I dont mind about this, as a compatibility shim can do simple mapping like this when the two libraries are close enough that a shim is sufficient.
There was a problem hiding this comment.
Regarding the capturing group name, I don't see any reason not to match a more sensible name, I will change that
Regarding the other rest commands, we only need to detail them, it makes sense to remove all of them if course
Regarding the branch name I've not come up with a working regexp to tell the branch name from the file path, as the branch name can contain slashes, more than happy to strip the branch from path and expose it as a variable
There was a problem hiding this comment.
pathname is now the same as git-url-parse, but I could not find a way to get the branch for the blob verb
tree verb instead can return the branch name
yakky
force-pushed
the
feature/strict-regexp
branch
from
70dea17 to
cb1b65d
Compare
|
@jayvdb I have been able to look at this only after so much time, unfortunately But I spent some time to add some git-url-parse compatibility |
yakky
force-pushed
the
feature/strict-regexp
branch
from
cb1b65d to
3045e95
Compare
Fix #18
Fix #10