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

Drop MacroTools dependency #15

Merged
merged 1 commit into from
Jan 16, 2020
Merged

Conversation

oxinabox
Copy link
Contributor

Its not worth depending on MacroTools and thus anything it depends on (DataStructures & OrderedCollections)

For this simple function.

I also think this implementation of the function is better,


[deps]
MacroTools = "1914dd2f-81c6-5fcd-8719-6d5c9610ff09"
version = "0.2.2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Last release was 0.2.1
so this is good to tag once this is merged

@devmotion
Copy link
Member

I also think this implementation of the function is better,

Do you mean this implementation of postwalk should be used by MacroTools as well?

@oxinabox
Copy link
Contributor Author

Do you mean this implementation of postwalk should be used by MacroTools as well?

I was mostly being facecious, because its a 2 line function so how can one do it "better".
Though I do fine MacroTool's harder to read because it abstracts prewalk and postwalk into a single walk inner function.

@devmotion
Copy link
Member

Yes, I agree that your implementation is definitely easier to understand 👍

@devmotion
Copy link
Member

@ChrisRackauckas, can you merge the PR? I don't have write access.

@ChrisRackauckas
Copy link
Member

oh you don't? I thought you did.

@ChrisRackauckas ChrisRackauckas merged commit 33b6e15 into SciML:master Jan 16, 2020
@devmotion
Copy link
Member

I was also surprised 😛

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