pik-piam / gms

Other
1 stars 15 forks source link

Make file distribution optional and better error message #72

Closed robinhasse closed 11 months ago

robinhasse commented 11 months ago
codecov[bot] commented 11 months ago

Codecov Report

Attention: 37 lines in your changes are missing coverage. Please review.

Comparison is base (f88533c) 34.96% compared to head (83c1f42) 34.71%.

:exclamation: Current head 83c1f42 differs from pull request most recent head 208905b. Consider uploading reports for the commit 208905b to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #72 +/- ## ========================================== - Coverage 34.96% 34.71% -0.25% ========================================== Files 51 51 Lines 1676 1688 +12 ========================================== Hits 586 586 - Misses 1090 1102 +12 ``` | [Files](https://app.codecov.io/gh/pik-piam/gms/pull/72?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pik-piam) | Coverage Δ | | |---|---|---| | [R/getfiledestinations.R](https://app.codecov.io/gh/pik-piam/gms/pull/72?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pik-piam#diff-Ui9nZXRmaWxlZGVzdGluYXRpb25zLlI=) | `0.00% <0.00%> (ø)` | | | [R/download\_distribute.R](https://app.codecov.io/gh/pik-piam/gms/pull/72?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pik-piam#diff-Ui9kb3dubG9hZF9kaXN0cmlidXRlLlI=) | `0.00% <0.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

orichters commented 11 months ago

Wouldn't the alternative simply be to return NULL here in getfiledestinations() and then make the distribution conditional on ! is.null(file2destination)? So if you don't find input files, don't distribute anything? You would not need the additional distribute switch anymore…

robinhasse commented 11 months ago

Wouldn't the alternative simply be to return NULL here in getfiledestinations() and then make the distribution conditional on ! is.null(file2destination)? So if you don't find input files, don't distribute anything?

Thats definitely smarter but changes the default behaviour (though only in case of a former error). I could add a warning that can be suppressed if you don't expect distribution.

orichters commented 11 months ago

I even think a message would be sufficient. If you would like to use the function as such in your model, then it shouldn't raise a warning.

robinhasse commented 11 months ago

I even think a message would be sufficient. If you would like to use the function as such in your model, then it shouldn't raise a warning.

I eventually did it the way you suggested. Thanks Oli!

robinhasse commented 11 months ago

I like the idea of the PR, it just needs some more finetuning (see my specific comments)

Thanks @tscheypidi for taking a look. You mentioned (multiple?) comments but I see just one. Do you have other concerns I should take care of?

tscheypidi commented 11 months ago

I made two comments, but was now also not able to see my second one. I was adding to Olivers comment with the messaging the suggestion not to trigger a message at all if not files are to be deleted, as this might be confusing to users not familiar with the function and is probably not necessary to be mentioned. Other than that I think I did not had any further comments