kbaseapps / DataFileUtil

MIT License
0 stars 12 forks source link

Clean up filename retrieval to prevent a couple potential bugs #76

Closed jayrbolton closed 3 years ago

jayrbolton commented 3 years ago

See this Jira ticket: https://kbase-jira.atlassian.net/browse/PUBLIC-1460

Before, filenames from URLs were getting fetched including url query params. This excludes the query params using urllib.

Also, we limit the filename to at most 255 characters, which I think is a reasonable maximum to avoid OSErrors.

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging 432ab7d7d1d697bd7bb67f918efe1ac5aeb25702 into e36bf01a491f1d9daa49fec44cec6112e870acba - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging 1e345af59d7bf41d3e401e846b528d0573febfe4 into 99129b3a673f34ca9da45c3e835bfa017ecb5e87 - view on LGTM.com

fixed alerts:

codecov[bot] commented 3 years ago

Codecov Report

Merging #76 (c364faf) into master (d4b72c5) will increase coverage by 0.37%. The diff coverage is 97.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   70.52%   70.89%   +0.37%     
==========================================
  Files           4        5       +1     
  Lines        1167     1182      +15     
==========================================
+ Hits          823      838      +15     
  Misses        344      344              
Impacted Files Coverage Δ
lib/DataFileUtil/DataFileUtilImpl.py 90.61% <91.66%> (-0.20%) :arrow_down:
lib/DataFileUtil/utils/retrieve_filename.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d4b72c5...2a4bdfd. Read the comment docs.

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging c1dfd51569d0a985aa65572d851041b2868ea0a5 into d4b72c51cd32c2f61173cd5e1e76f683b4ca4f04 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging e401d82684e6d999d85785d64810d5bdeb65a91d into d4b72c51cd32c2f61173cd5e1e76f683b4ca4f04 - view on LGTM.com

fixed alerts:

dcchivian commented 3 years ago

@jayrbolton @MrCreosote this PR is still hanging about. should we close it out?

jayrbolton commented 3 years ago

@dcchivian I still plan to add some additional tests as Gavin requested. It's pretty high up in my todo list. Then we should merge this, not close it, as it addresses a bug.

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging d44d0c5d10e128a6aa921ccdec08dbed86189872 into f504ca336765046f39d92c8b9d21e75129b1dd10 - view on LGTM.com

fixed alerts:

scanon commented 3 years ago

What's needed to close this out?

jayrbolton commented 3 years ago

I need to finish the tests. I will push the updates soon. I haven't been working on it in favor of KE, search, and RE stuff.

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging 447e559bab6a476db01492a62817389ca6780f78 into f504ca336765046f39d92c8b9d21e75129b1dd10 - view on LGTM.com

fixed alerts:

dcchivian commented 3 years ago

@jayrbolton @MrCreosote @sychan @scanon @Yzm1234 the GHA tests are failing. I think it's probably @jayrbolton who needs to give it some TLC?

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging f4cdb5b9fe1a80b4d2c32dc2b9f96e6cb9e8a471 into f504ca336765046f39d92c8b9d21e75129b1dd10 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging 71a20ac041d4d63e247dc8732980af5fffdcc16f into f504ca336765046f39d92c8b9d21e75129b1dd10 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging de993b7667e1c29510fa640310e9778f3802bbfd into f504ca336765046f39d92c8b9d21e75129b1dd10 - view on LGTM.com

fixed alerts:

MrCreosote commented 3 years ago

Hmm, adding comments to a prior review in a new review makes things confusing

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging 2a4bdfd1c68ec7c77e1acf9644d4d66cbf4237c4 into f504ca336765046f39d92c8b9d21e75129b1dd10 - view on LGTM.com

fixed alerts:

jayrbolton commented 3 years ago

@Tianhao-Gu @jsfillman @qzzhang This PR is ready to merge, but one of you 3 need to approve before I can do so because you're in the CODEOWNERS file