Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support native pathlike objects #105

Merged
merged 1 commit into from
Jan 31, 2022
Merged

Support native pathlike objects #105

merged 1 commit into from
Jan 31, 2022

Conversation

alecandido
Copy link
Member

This should close #84.

It's a tiny PR, and we can even quickly merge.
I ran existing tests, and slightly extended (sorry for the diff, but someone forgot to run black on unit tests).
I even tested it manually, and it's smoothly working.

Speaking of tests: maybe it might be a good chance to extend the IO related ones a bit (write_lz4 is not tested, nor read, write and write_lz4 for FkTables... actually almost nothing is tested of Fk).
In any case: the interface is a thin wrapper, so an extensive test suite is not that required, and we can even postpone to a more consistent tests rework.

@codecov
Copy link

codecov bot commented Jan 28, 2022

Codecov Report

Merging #105 (5c22c2f) into master (c0144e4) will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
+ Coverage   88.40%   88.46%   +0.06%     
==========================================
  Files          31       31              
  Lines        3070     3070              
==========================================
+ Hits         2714     2716       +2     
+ Misses        356      354       -2     
Impacted Files Coverage Δ
pineappl_cli/src/plot.rs 93.15% <0.00%> (+1.05%) ⬆️

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 c0144e4...5c22c2f. Read the comment docs.

@alecandido alecandido requested a review from cschwan January 29, 2022 10:27
@cschwan
Copy link
Contributor

cschwan commented Jan 31, 2022

Looks good to me!

@cschwan
Copy link
Contributor

cschwan commented Jan 31, 2022

Speaking of tests: maybe it might be a good chance to extend the IO related ones a bit (write_lz4 is not tested, nor read, write and write_lz4 for FkTables... actually almost nothing is tested of Fk).
In any case: the interface is a thin wrapper, so an extensive test suite is not that required, and we can even postpone to a more consistent tests rework.

Agreed, I've opened Issue #108 for that. Thanks for your work on this Issue @alecandido!

@cschwan cschwan closed this Jan 31, 2022
@cschwan cschwan reopened this Jan 31, 2022
@cschwan cschwan merged commit 2ad48d8 into master Jan 31, 2022
@cschwan cschwan deleted the py-paths branch January 31, 2022 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python paths handling
2 participants