-
Notifications
You must be signed in to change notification settings - Fork 111
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
Tracking non-.jl source files #680
Conversation
It would be good to document what are the exact methods one need to overload in order to hook up a custom file format to Revise. And then to finish it off, add a test as well. |
(fatfingered the close button) |
Should I add a section to the developer reference with details on how to do this? |
I think that is a good place, yes. |
Just so I don't forget, here is a relatively minimal example of using this feature
|
@KristofferC can you re-run CI? |
for reference, this depends on timholy/CodeTracking.jl#95 |
@timholy bump on this? What are your thoughts? |
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.
This seems fine in principle, with the major concern being timholy/CodeTracking.jl#95 (comment).
Briefly, is the idea that this tracks changes in, e.g., Python code as you develop? Wouldn't you be testing the Python code in Python first? I'm a bit confused about exactly how this will be handy (not doubtful, just confused).
src/packagedef.jl
Outdated
topmod = first(keys(mexsold)) | ||
fileok = file_exists(filep) | ||
fileok = file_exists(String(filep)) |
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.
If you're doing it this way, any reason not to use to insulate this from invalidation, string
instead of String
? Conversely,String(filep)::String
might be necessary.
The use case for this is in X->julia transpilers. Revising the foreign code is useful because it allows you to use the Julia repl (including debugger.jl) as a repl for non julia source code which can be very useful especially when X is a language that doesn't have good tooling (hence why you are writing a transpiler in the first place). This wouldn't be that useful for Python or other major languages (they tend to be big and have good tooling anyway), but for small languages that traditionally don't have good tooling (i.e. C/BrainFuck/random proprietary languages people wrote in the 80s) |
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.
This mostly looks good. I'm proposing a couple of changes to decrease the risk of invalidation of Revise's own code when people load packages.
We do need the test working before we can merge this. I think you also need to run this test from runtests.jl
?
One complication here is that these tests won't pass until timholy/CodeTracking.jl#95 is merged and tagged. |
Update src/pkgs.jl Co-authored-by: Kristoffer Carlsson <kcarlsson89@gmail.com> add docs add non_julia program Create non_jl_test.jl Update types.jl Update types.jl simplify simplify requirements. update docs update test Create new_test.program test revising Update test/fake_lang/non_jl_test.jl Co-authored-by: Tim Holy <tim.holy@gmail.com> fix typo in tests test fake_lang update tests
Invalidations fixed. Tests pass (with the codetracking PR+Julia1.7. On 1.9, the tests fail with or without this PR, but including my test file passes). |
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.
There still seems to be some unnecessary invalidation risk here. Realized invalidations depend on which package you load; you might try InlineStrings.jl as a package that defines a lot of new string types and methods, e.g.,
using SnoopCompileCore
using Revise # Revise precompiles a lot, so you're probably OK, otherwise force a revision to make sure everything is compiled before the next step
invs = @snoopr using InlineStrings
using SnoopCompile
trees = invalidation_trees(invs)
apologies. the things I resolved were changes that I made that I then lost while rebasing (and didn't check) |
Co-authored-by: Tim Holy <tim.holy@gmail.com>
Co-authored-by: Tim Holy <tim.holy@gmail.com>
Co-authored-by: Tim Holy <tim.holy@gmail.com>
Co-authored-by: Tim Holy <tim.holy@gmail.com>
That fixed the first path issue but there are more. Presumably it fails locally in the same way with |
No, this is passing locally (even if I don't run the tests from the Revise folder). |
Co-authored-by: Sebastian Pfitzner <pfitzseb@gmail.com>
OK. Then try the same trick with all the rest of your paths? |
You have another one on line 42? |
Co-authored-by: Sebastian Pfitzner <pfitzseb@gmail.com>
Non-nightly tests seem to pass now. |
Thanks so much! |
This PR removes the requirement that
file
be a::AbstractString
which makes it possible to teach Revise about non-julia source files by making your own file type (egPyFile
).