mathause / filefinder

find and parse file and folder names
MIT License
3 stars 1 forks source link

on_missing ignore to skip and foo #79

Closed veni-vidi-vici-dormivi closed 7 months ago

veni-vidi-vici-dormivi commented 7 months ago

Closes #77 Change option "ignore" to "skip" in on_missing to be consistent with on_parse_error (see #75). Implement behavior if on_missing is none of the implemented options.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.85%. Comparing base (09d212b) to head (83dc9c7). Report is 9 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #79 +/- ## ========================================== - Coverage 88.05% 87.85% -0.21% ========================================== Files 5 5 Lines 268 280 +12 ========================================== + Hits 236 246 +10 - Misses 32 34 +2 ``` | [Flag](https://app.codecov.io/gh/mathause/filefinder/pull/79/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Mathias+Hauser) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/mathause/filefinder/pull/79/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Mathias+Hauser) | `87.85% <100.00%> (-0.21%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Mathias+Hauser#carryforward-flags-in-the-pull-request-comment) to find out more.

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

veni-vidi-vici-dormivi commented 7 months ago

The check for the correct keyword is often done outside the inner if to fail early

Of course, that makes sense. Would you still put it in the function where it is actually used or in the wrapper?

mathause commented 7 months ago

Would you still put it in the function where it is actually used or in the wrapper?

Here, I would put it in priority_filter (however, I don't want to generalize. If _prioritize was used in several places its prob. better to put it there)

veni-vidi-vici-dormivi commented 7 months ago

Actually in on_parse_error the options are 'raise', 'warn' and 'skip'. I guess we should change 'error' to 'raise' here too? 😅

mathause commented 7 months ago

Actually in on_parse_error the options are 'raise', 'warn' and 'skip'. I guess we should change 'error' to 'raise' here too? 😅

Good catch. I quickly had a look (in xarray) and "raise" seems the default choice.

(I also found that they rather use "ignore" instead of "skip", so I am considering if we really should ~not rather~ use "skip" here)

veni-vidi-vici-dormivi commented 7 months ago

Well well well then let's go back to ignore and change error to raise. Also, I'll open another branch for the analogous parse_error redo.

veni-vidi-vici-dormivi commented 7 months ago

But for parse_error skip -> ignore we don't need a FutureWarning because we didn't release it yet, is that right?

mathause commented 7 months ago

Can you merge main into this PR? Maybe you'll get a merge conflict :rocket: