[WIP] Add support for converting PURLS to URLS - #27
Conversation
TG1999
changed the title
TG1999
changed the title
|
@pombredanne @MaJuRG please review :) |
|
|
||
| def build_bitbucket(name, namespace, version, qualifiers, subpath): | ||
|
|
||
| if name and namespace: |
There was a problem hiding this comment.
Why not check the inverse of this and return right away? This would remove this big indented block.
There was a problem hiding this comment.
What do you mean by inverse :), please can you explain a little for me :D
There was a problem hiding this comment.
for example, you would do
if not name or not namespace:
return
# rest of code at normal indentation level.
There was a problem hiding this comment.
Ohhkay, thanks got it
|
|
||
|
|
||
| def build_cargo(name, namespace, version, qualifiers, subpath): | ||
| if name and version: |
There was a problem hiding this comment.
Why not check the inverse of this and return right away? This would remove this big indented block.
|
|
||
| def build_github(name, namespace, version, qualifiers, subpath): | ||
|
|
||
| if name and namespace: |
There was a problem hiding this comment.
Why not check the inverse of this and return right away? This would remove this big indented block.
|
|
||
|
|
||
| def build_rubygems(name, namespace, version, qualifiers, subpath): | ||
| if name and version: |
There was a problem hiding this comment.
Why not check the inverse of this and return right away? This would remove this big indented block.
| "pkg:github/tg1999/fetchcode": "https://github.com/tg1999/fetchcode", | ||
| "pkg:github/tg1999/fetchcode@master": "https://github.com/tg1999/fetchcode/tree/master", | ||
| "pkg:github/tg1999/fetchcode@master#tests": "https://github.com/tg1999/fetchcode/tree/master/tests", | ||
| "pkg:github/tg1999": None, | ||
| "pkg:cargo/clap@2.3.3": "https://crates.io/api/v1/crates/clap/2.3.3/download", | ||
| "pkg:cargo/rand@0.7.2": "https://crates.io/api/v1/crates/rand/0.7.2/download", | ||
| "pkg:cargo/structopt@0.3.11": "https://crates.io/api/v1/crates/structopt/0.3.11/download", |
There was a problem hiding this comment.
We probably want to have test cases that contain all possible combos of purl fields for these tests.
Such as purls with namespace, subpath, qualifiers etc
steven-esser
left a comment
steven-esser
left a comment
There was a problem hiding this comment.
@TG1999 This is looking very good. Just see a few nitpicking details I have left.
| return url | ||
|
|
||
|
|
||
| def build_cargo(name, namespace, version, qualifiers, subpath): |
There was a problem hiding this comment.
Why do we only use a subset of the arguments here to build a url?
There was a problem hiding this comment.
Can you please explain a little bit, what should be done here, it will be helpful for me 😅
There was a problem hiding this comment.
This function accepts 5 different arguments, but you only use a subset of these to build the URL. You need to either:
- use all the args in some way
- remove extraneous args that aren't used
It is confusing otherwise
There was a problem hiding this comment.
Okay I am getting your point :), I am passing a kwarg from the purl2url function and unpacking it here, can you suggest me a better alternative, I was also finding some way for that, but not able to do so
There was a problem hiding this comment.
Just make the function accept kwargs. See: https://www.digitalocean.com/community/tutorials/how-to-use-args-and-kwargs-in-python-3
There are other resources out there as well.
| ) | ||
|
|
||
|
|
||
| def build_github(name, namespace, version, qualifiers, subpath): |
There was a problem hiding this comment.
Why do we only use a subset of the arguments here to build a url?
| return url | ||
|
|
||
|
|
||
| def build_gitlab(name, namespace, version, qualifiers, subpath): |
There was a problem hiding this comment.
Why do we only use a subset of the arguments here to build a url?
| return url | ||
|
|
||
|
|
||
| def build_gem(name, namespace, version, qualifiers, subpath): |
There was a problem hiding this comment.
Why do we only use a subset of the arguments here to build a url?
| subpath = url_parts.fragment if url_parts.fragment != "" else None | ||
| qualifiers = url_parts.query if url_parts.query != "" else None | ||
|
|
||
| path = url_parts.path |
There was a problem hiding this comment.
Can we use a more descriptive name for path here? what kind of path is it?
| return url | ||
|
|
||
|
|
||
| def build_cargo(name, namespace, version, qualifiers, subpath): |
There was a problem hiding this comment.
Can we add some descriptive docstring for this function
| ) | ||
|
|
||
|
|
||
| def build_github(name, namespace, version, qualifiers, subpath): |
There was a problem hiding this comment.
Can we add some descriptive docstring for this function
| return url | ||
|
|
||
|
|
||
| def build_gitlab(name, namespace, version, qualifiers, subpath): |
There was a problem hiding this comment.
Can we add some descriptive docstring for this function
| return url | ||
|
|
||
|
|
||
| def build_gem(name, namespace, version, qualifiers, subpath): |
There was a problem hiding this comment.
Can we add some descriptive docstring for this function
| ) | ||
|
|
||
|
|
||
| def purl2url(purl): |
There was a problem hiding this comment.
Can we add some descriptive docstring for this function
Signed-off-by: TG1999 <tushar.goel.dav@gmail.com>
pombredanne
left a comment
pombredanne
left a comment
There was a problem hiding this comment.
Thank you!
- I think this should be in the packageurl library instead
- you are missing unit-level test for several functions.
| """ | ||
|
|
||
| name = data["name"] | ||
| namespace = data["namespace"] |
There was a problem hiding this comment.
Please use single quotes throughout unless this is a docstring
There was a problem hiding this comment.
I ran black, due to which all these single quotes converted into double quotes, apologies for that 😅
There was a problem hiding this comment.
That's great then. Stick to black and make it part of the CI and tests too.
| from urllib.parse import urlparse | ||
|
|
||
|
|
||
| def index_in_list(index, list): |
There was a problem hiding this comment.
Do not create a function for something so trivial, also in general using indexes on a list is often a sign that there must be a better way to do the work.
|
|
||
| def build_bitbucket(data): | ||
| """ | ||
| Take dictionary `data` as input and returns a valid bitbucket URL string `url` |
There was a problem hiding this comment.
Use imperative style for docstrings.
Return a Bitbucket URL from a mapping of Package URL data
But there is a larger problem... why use a dict here rather than the packageurl library and object that has nice named attributes?
Also should this be code that lives in the packageurl library instead?
| """ | ||
| Take PackageURL `purl` as input and return a valid URL string `url` depending on the type of purl | ||
| """ | ||
| url_parts = urlparse(purl) |
There was a problem hiding this comment.
Why parsing a purl yourself? please use the packageurl library instead
| name = path[1] | ||
|
|
||
| type = path[0] | ||
|
|
There was a problem hiding this comment.
All the code above is parsing a purl which already exists in a tested and more robust form in the packaeurl library. Please use that library instead.
| from fetchcode.purl2url import purl2url | ||
|
|
||
|
|
||
| def test_convert_with_purls_string(): |
There was a problem hiding this comment.
purl2url should be part of the packageurl library IMHO
|
@TG1999 why did you close this? can you explain? |
|
I have shifted it to packageul-python as suggested by you |
3 participants
Signed-off-by: TG1999 tushar.goel.dav@gmail.com