simonster / Reexport.jl

Julia macro for re-exporting one module from another
Other
161 stars 19 forks source link

Make Reexport composable with other import macros #30

Closed lassepe closed 3 years ago

lassepe commented 3 years ago

This pull-request is an attempt to make Reexport composable with other import macros. This idea originated from https://github.com/Roger-luo/FromFile.jl/issues/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
codecov-commenter commented 3 years ago

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%) :arrow_up:

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.

lassepe commented 3 years ago

Should I also add some examples to the README?

ararslan commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

I agree 👍

lassepe commented 3 years ago

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 :)