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

Eliminate need to parse include paths #49

Merged
merged 7 commits into from Nov 22, 2017
Merged

Eliminate need to parse include paths #49

merged 7 commits into from Nov 22, 2017

Conversation

timholy
Copy link
Owner

@timholy timholy commented Sep 28, 2017

This is based on the not-yet-merged JuliaLang/julia#23898. Locally it passes all tests until it gets to the Revise.track tests, because there I haven't expunged the dependence on parsing. I'm posting this now because there seems to be room to discuss how this should work:

  • Revise.track(Base): perhaps the best approach here would be for base/sysimg.jl to keep track of the files it includes, and Revise could just look that information up.
  • More interesting is Revise.track(filename), which I'm thinking of in the context of "scripts" for test or analysis code. Currently we recursively track any file included by that file, based on parsing. If we wanted to replicate that, note that the new Revise.included_files list includes the entire set, once filename has been included once. The trouble is that if the user has included several files, there doesn't seem to be a great way to know which files got recursively included by which operation. One approach would be to temporarily empty the list and re-include filename, but that has some obvious downsides. Overall, here I favor restricting it to be literal, and require users to list each file they want to track.

Once merged this will fix #40.

@cstjean
Copy link
Collaborator

cstjean commented Oct 8, 2017

How will this interact with @require? AFAICT @require A include("foo.jl") will correctly track foo.jl, but @require A function bar() ... end will not track bar. Is that right?

@timholy
Copy link
Owner Author

timholy commented Oct 8, 2017

Not sure, to be honest.

FYI once I finish closing a couple of other issues, I'm planning to tag a v0.1.0 release, fork a 0.6 branch, and then make master 0.7-only. JuliaLang/julia#23898 makes Revise work so much more reliably, I think we really should just focus on making Revise as good as it can be for Julia 1.0.

When tagged, the new one will be (at least) v0.2.0, so that if needed we can continue to fix 0.6 on that 0.6 branch.

Seem OK to you?

@cstjean
Copy link
Collaborator

cstjean commented Oct 8, 2017

Sure, I will most likely do the same with TraceCalls.

@timholy
Copy link
Owner Author

timholy commented Oct 8, 2017

Great. Just tagged 0.1.

I may have just figured out in principle how to handle deleted specializations. For me I think that's the main reason for Revise to "fail", so I'm tempted to see if I can get that in before the fork. Won't be quick, though, I have other deadlines (the PRs were a "study break" :smile).

@timholy timholy changed the title WIP/RFC: eliminate need to parse include paths Eliminate need to parse include paths Oct 11, 2017
@timholy timholy force-pushed the teh/callbacks branch 3 times, most recently from 61e2c7d to 225d186 Compare October 11, 2017 10:39
@timholy timholy closed this Oct 11, 2017
@timholy timholy reopened this Oct 11, 2017
@codecov
Copy link

codecov bot commented Oct 11, 2017

Codecov Report

Merging #49 into master will decrease coverage by 1.49%.
The diff coverage is 93.63%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #49     +/-   ##
=========================================
- Coverage   85.48%   83.98%   -1.5%     
=========================================
  Files           1        4      +3     
  Lines         310      306      -4     
=========================================
- Hits          265      257      -8     
- Misses         45       49      +4
Impacted Files Coverage Δ
src/types.jl 100% <100%> (ø)
src/parsing.jl 89.47% <89.47%> (ø)
src/relocatable_exprs.jl 95.45% <95.45%> (ø)
src/Revise.jl 79.7% <96.22%> (-5.79%) ⬇️

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 90d49fa...6349790. Read the comment docs.

@timholy
Copy link
Owner Author

timholy commented Oct 20, 2017

This should work on sufficiently-recent master, but until CI passes I'm reluctant to merge since I can only test Linux. JuliaLang/julia#23846

This is a major redesign:
- requires julia-0.7
- relies on new include callbacks and extra information stored in *.ji files to get the list of files to track
- splits code base into multiple files for easier navigation and understanding
- adopts the Base.root_module interface for working with modules
@timholy timholy merged commit 0aba742 into master Nov 22, 2017
@timholy timholy deleted the teh/callbacks branch November 22, 2017 18:16
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.

Packages with fancy include mechanisms
2 participants