Add npm source #296
Add npm source #296
Conversation
Codecov Report
@@ Coverage Diff @@
## master #296 +/- ##
===========================================
+ Coverage 15.92% 28.28% +12.35%
===========================================
Files 6 6
Lines 427 442 +15
===========================================
+ Hits 68 125 +57
+ Misses 359 317 -42
Continue to review full report at Codecov.
|
| name = "npm" | ||
|
|
||
| def get_url(self, meta_yaml): | ||
| if "registry.npmjs.org" in meta_yaml["url"]: |
There was a problem hiding this comment.
Should this be not in?
|
|
||
| def get_version(self, url): | ||
| r = requests.get(url) | ||
| latest = r.json()["dist-tags"].get("latest", "").strip() |
There was a problem hiding this comment.
Can you please check r.ok before trying to get the json?
|
Thanks for putting this in, @bollwyvl! |
|
Instead of checking the url to have |
Hmmm... I guess the namespace thing is what trips me up from taking that approach. A separate PR could look to doing something about the github release filtering as well, but i have no data outside of a bit of looking as to which projects are aggressively using that particular ui feature... i guess if they ever used a pre-release, then you'd only want the stable releases. |
|
Thanks for putting this in @bollwyvl! Generally LGTM |
|
It would be nice to have tests for this, but we don't have any other tests for related classes, so I don't feel comfortable putting that burden on this PR. But it would make a nice addition 😉 |
|
Oh no worries, tests sound good!
…On Fri, Jul 27, 2018 at 9:19 AM Anthony Scopatz ***@***.***> wrote:
It would be nice to have tests for this, but we don't have any other tests
for related classes, so I don't feel comfortable putting that burden on
this PR. But it would make a nice addition 😉
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#296 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AACxRJkbL_vVT9D_yaxv-dF2xrgkVsjYks5uKxNMgaJpZM4Vivnb>
.
|
|
I'm 👍 on these changes. Tests would be spectacular! |
|
Added a simple test with requests-mock. I kinda phoned it in on the |
|
|
||
| @pytest.mark.parametrize("inp, ver, source, urls", latest_url_test_list) | ||
| def test_latest_version(inp, ver, source, urls, requests_mock): | ||
| pmy = parse_meta_yaml(inp)["source"] |
There was a problem hiding this comment.
What's the right way to transform the package into the thing that get_latest_version is expecting?
There was a problem hiding this comment.
You could use get_attrs https://github.com/regro/cf-scripts/blob/master/conda_forge_tick/make_graph.py#L24 (although this currently pulls the meta.yaml itself from github. I guess at some point we should separate the request from the rest of it for these kinds of things.
There was a problem hiding this comment.
Hm, guess I can mock that, too...
|
Pinging this issue. @CJ-Wright @justcalamari - should this go in as is? |
|
@bollwyvl thank you very much! |
Fixes #291.
This adds a simple, yet not very widely applicable, source to, if the feedstock url is from
registry.npmjs.org, make a call toregistry.npmjs.organd return thedist-tagwhich islatest(the default). Like the PyPI source, it also will not return pre-release-y values in the name of the tag, as that also happens as thedist-tagsfeature is fairly buried.Not doing prereleases might be introducing an unwelcome opinion... thoughts? Both of the major upstream tools that use the namespace,
npmandyarn, ignore them, so it seems reasonable enough.