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

WIP: Depend on ExprTools.jl #132

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

WIP: Depend on ExprTools.jl #132

wants to merge 3 commits into from

Conversation

omus
Copy link
Contributor

@omus omus commented Jan 17, 2020

A while ago I re-implemented splitdef/combinedef to handle a larger variety of function definition as well as get better performance. I originally just added these functions directly to Mocking.jl as I was trying to minimize dependencies in that package. There is now some interest in using these revised functions outside of Mocking so I thought a new package which provides the lightweight parts of MacroTools.jl may be the best option here.

To that end this PR would change MacroTools.jl to depend on ExprTools.jl to avoid duplicating efforts. I could also see having both variations co-exist in separate packages. This PR is a WIP as two things need to occur:

@nickrobinson251
Copy link

Bump :)

@cstjean
Copy link
Collaborator

cstjean commented Mar 21, 2020

I wonder how many projects are big enough to use splitdef, but won't have an indirect dependency on DataStructures, which makes up most of MacroTools's loading time on my machine.

If we're taking it out, then we might as well remove it entirely (deprecate it?) and redirect people to ExprTools instead of adding a dependency to this package. Then we don't have to keep the MacroTools README in sync with the ExprTools README.

Furthermore, there is also MacroTools.splitarg and MacroTools.splitstructdef (written by someone else). It doesn't make sense to split them apart. We'd have to move/reimplement them in ExprTools.

I'm the original author of splitdef. 🤷‍♂ I'm fine either way.

@omus
Copy link
Contributor Author

omus commented May 25, 2020

Since #139 removes the DataStructures dependency I'd be okay with moving the ExprTools functionality here entirely as this package is now light with its dependencies. However, I'm not sure MacroTools will guarantee it will be light with its dependencies in the future.

@MikeInnes I'd like to know your opinion on this.

@MikeInnes
Copy link
Member

Yes, I think MacroTools is unlikely to add major new features at this point, and since some people clearly care about keeping dependencies few it would be fine to add that guarantee to the package.

@fingolfin
Copy link
Contributor

What's the status of this then?

@cstjean cstjean mentioned this pull request Dec 31, 2021
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.

5 participants