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

Make Reexport composable with other import macros #30

Merged
merged 11 commits into from
Aug 21, 2021

Conversation

lassepe
Copy link
Contributor

@lassepe lassepe commented Aug 8, 2021

This pull-request is an attempt to make Reexport composable with other import macros. This idea originated from Roger-luo/FromFile.jl#25

In order to facilitate compatibility with FromFile.jl (and potentially other import macro tools) this PR introduces the following change:

  1. Any macros in the incoming expression are expanded via Base.macroexpand, thus @reexport always sees the code as if all the syntax transformations have been written out
  2. @reexport recursively expands all :block expressions (since macros like FromFile.@from return a block of multiple import statements)
  3. Reexport is expanded to also handle statements of the form @reexport import A.b and @reexport import A: b which are transformed to import A.b; export b and import A: b; export b respectively.

Please let me know what you think about this proposal. I will add tests, once we've worked out if we want this feature at all. Also, please let me know if you prefer this pull quest to be split up into multiple smaller ones.

Edit:

The feature number 2 above has the nice side-effect that you can now wrap an entire block in a @reexport macro:

@reexport begin
    using A: b
    import C.d
end

@lassepe lassepe marked this pull request as draft August 8, 2021 15:43
@lassepe lassepe marked this pull request as ready for review August 10, 2021 20:48
@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2021

Codecov Report

Merging #30 (a3e7e23) into master (3ab612e) will increase coverage by 14.28%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           master       #30       +/-   ##
============================================
+ Coverage   85.71%   100.00%   +14.28%     
============================================
  Files           1         1               
  Lines          14        20        +6     
============================================
+ Hits           12        20        +8     
+ Misses          2         0        -2     
Impacted Files Coverage Δ
src/Reexport.jl 100.00% <100.00%> (+14.28%) ⬆️

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 3ab612e...a3e7e23. Read the comment docs.

Copy link
Collaborator

@ararslan ararslan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work! Just a couple of comments but overall I like this idea.

@lassepe lassepe force-pushed the feature/fromfile_compat branch from 303b776 to fd52f68 Compare August 21, 2021 20:44
@lassepe
Copy link
Contributor Author

lassepe commented Aug 21, 2021

Should I also add some examples to the README?

@ararslan
Copy link
Collaborator

I've leave that up to you whether you want to add some examples. Examples are great but I'm also fine accepting this PR as-is.

@lassepe
Copy link
Contributor Author

lassepe commented Aug 21, 2021

Okay cool. Please don't merge this yet. I think I found another edge-case regarding for import Foo: bar. Currently, only import Foo.bar is supported

@lassepe lassepe marked this pull request as draft August 21, 2021 21:53
src/Reexport.jl Outdated
error("@reexport: syntax error")

if ex.head == :module
modules = Any[ex.args[2]]
ex = Expr(:toplevel, ex, :(using .$(ex.args[2])))
elseif ex.head == :using && all(e->isa(e, Symbol), ex.args)
elseif ex.head == :using && all(e -> isa(e, Symbol), ex.args)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ararslan I think this branch can never be reached? In fact, I removed it and all tests still pass. Do you know more?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right, good catch. This package is very old—it was written for Julia 0.2, I believe—and the way some of these expressions are structured internally has since changed but not all of the code here has been updated to match. (I thought it had been but clearly not!) AFAICT, this branch would have accepted :(using A) and :(using A.B) in earlier Julia versions. As you've noticed, it seems now to be dead code.

@lassepe
Copy link
Contributor Author

lassepe commented Aug 21, 2021

Another thing that I am wondering: What is the desired behavior of @reexport import Foo. Currently it only exports Foo not the names within. In my opinion this is the correct behavior because import Foo also only adds Foo to the current namespace. Or should it be disallowed?

@lassepe lassepe marked this pull request as ready for review August 21, 2021 22:15
@lassepe
Copy link
Contributor Author

lassepe commented Aug 21, 2021

I added examples to the README and also handled @reexport import Foo: bar. As for @reexport import Foo I think the behavior of not exporting any of Foo's names is correct, so I did not change that. Thus, from my side this would be ready for another review.

@ararslan
Copy link
Collaborator

In my opinion this is the correct behavior because import Foo also only adds Foo to the current namespace.

I agree 👍

@lassepe lassepe force-pushed the feature/fromfile_compat branch from c4a1f66 to 2fa254a Compare August 21, 2021 22:52
Copy link
Collaborator

@ararslan ararslan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work and thanks for your patience with my review 😄

@ararslan ararslan merged commit 4398589 into simonster:master Aug 21, 2021
@lassepe
Copy link
Contributor Author

lassepe commented Aug 21, 2021

Great work and thanks for your patience with my review 😄

Thank you for the detailed review! It's my first time meta programming so it's good to get a lot of feedback :)

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.

3 participants