-
Notifications
You must be signed in to change notification settings - Fork 13
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
Sourcefile does not have a filepath #177
Conversation
joscao
commented
Oct 23, 2023
- sourcefile has a source not a filepath
- sourcefile has a source not a filename
Codecov Report
@@ Coverage Diff @@
## main #177 +/- ##
=======================================
Coverage 92.14% 92.14%
=======================================
Files 90 90
Lines 16690 16690
=======================================
Hits 15379 15379
Misses 1311 1311
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's a nice find. Any chance we could test this behaviour in a test, since it's clearly not covered at the moment?
Also, I think the property to be used is Sourcefile.path
rather than source
.
tests/conftest.py
Outdated
@@ -71,7 +71,7 @@ def jit_compile(source, filepath=None, objname=None): | |||
Return a specific object (module or subroutine) in :attr:`source` | |||
""" | |||
if isinstance(source, Sourcefile): | |||
filepath = source.filepath if filepath is None else Path(filepath) | |||
filepath = source.source if filepath is None else Path(filepath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, Sourcefile.source
is an object of type Source
rather than a path. However, the current implementation is also wrong, it should be using source.path
rather than source.filepath
:
Line 60 in af8ec9f
self.path = Path(path) if path is not None else path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used path instead of source. bc8f27b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discovered that filepath can still be empty, therefore is starting to write in ".", avoid this by using tempdir
87bc181
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, also for the added handling of empty filepaths. Looks great now!