Closed kkovary closed 1 year ago
Merging #212 (4bf712c) into main (22b5bb0) will decrease coverage by
0.04%
. The diff coverage is77.77%
.
@@ Coverage Diff @@
## main #212 +/- ##
==========================================
- Coverage 91.95% 91.91% -0.04%
==========================================
Files 46 46
Lines 3827 3835 +8
==========================================
+ Hits 3519 3525 +6
- Misses 308 310 +2
Flag | Coverage Δ | |
---|---|---|
unittests | 91.91% <77.77%> (-0.04%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Files Changed | Coverage Δ | |
---|---|---|
datamol/data/__init__.py | 78.07% <75.00%> (-0.24%) |
:arrow_down: |
datamol/mol.py | 97.00% <100.00%> (ø) |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
Thank you for the catch! This is indeed a bug, but I don't think we should even open that file to get its path. Instead we should simply retrieve the path.
In datamol/data/__init__.py
, can you add the below function and use it in mol.py
with SALT_SOLVENT_PATH = datamol_data_file_path("salts_solvents.smi")
?
@functools.lru_cache()
def datamol_data_file_path(filename: str, dm_module: str = "datamol.data"):
if sys.version_info < (3, 9, 0):
with importlib_resources.path("datamol.data", filename) as p:
data_path = p
else:
data_path = importlib_resources.files(dm_module).joinpath(filename)
return str(data_path)
Thanks for the suggestion @hadim, that's definitely a much better way of handling things. I'll push an update a little later today.
@hadim I just pushed a commit with the requested changes.
Thank you, @kkovary, for your contribution!
Thank you @hadim for the wonderful tools, hope to make more contributions in the future 😄
Changelogs
closes #211
This line opens a file but does not close it, which is why the ResourceWarning was being raised. To resolve this issue, I've modified the line to use a with block, which ensures that the file is properly closed after its name is retrieved:
This change should prevent the ResourceWarning from being raised in the future. I've tested the change locally and confirmed that it resolves the warning without causing any other issues.
Checklist:
feature
,fix
ortest
(or ask a maintainer to do it for you).discussion related to that PR