Skip to content

Don't export flatten #1735

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Don't export flatten #1735

wants to merge 1 commit into from

Conversation

ChrisRackauckas
Copy link
Member 

julia> WARNING: both Flux and Iterators export "flatten"; uses of it in module DiffEqFlux must be qualified

Yeah no, that interferes with a very common Base module.

@ChrisRackauckas
Don't export flatten
a920f5d 
```julia
julia> WARNING: both Flux and Iterators export "flatten"; uses of it in module DiffEqFlux must be qualified
```

Yeah no, that interferes with a very common Base module.
@ToucheSir
Copy link
Member

Given flatten has been exported for over 2 and a half years now, this would have to be a breaking change.

@ChrisRackauckas
Copy link
Member Author

Could anyone have ever actually used this though? If one package in the entire stack uses Iterators it's going to have an issue. I've never seen a package export something that shadows Base 😅

@ToucheSir
Copy link
Member

It's come up a few times in MWEs in the wild. I imagine this hasn't come up already because most folks use Iterators functions off the module. I don't think I've ever seen using .Iterators before 🤷

DhairyaLGandhi
DhairyaLGandhi approved these changes Oct 5, 2021
View reviewed changes
Copy link
Member

@DhairyaLGandhi DhairyaLGandhi left a comment

Choose a reason for hiding this comment

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

Breaking or not, this is something we would need to address. Unexporting would also require some changes to docs and tests and such. @ChrisRackauckas lmk if you want me to take it forward.

@ToucheSir
Copy link
Member

Oh don't get me wrong, we shouldn't be exporting this. However, I don't think we should make this change until we're ready to cut a breaking release. It's a super trivial one, so as long as someone remembers we'll be good to go.

@ToucheSir ToucheSir added this to the v0.13 milestone Oct 5, 2021
@DhairyaLGandhi
Copy link
Member

flatten is used. It clashing with a Base submodule is unfortunate.

@CarloLucibello
Copy link
Member

I don't see many breaking changes on the horizon, right? We could just unexport flatten and params, remove a few deprecations and tag v0.13

@CarloLucibello CarloLucibello mentioned this pull request Oct 17, 2021
Merged
@CarloLucibello
Copy link
Member

Closing this in favor of #1751

@CarloLucibello CarloLucibello deleted the flatten branch April 7, 2022 07:04
@ToucheSir ToucheSir mentioned this pull request Sep 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DhairyaLGandhi DhairyaLGandhi DhairyaLGandhi approved these changes

Assignees
No one assigned
Labels
breaking
Projects
None yet
Milestone
v0.13
Development

Successfully merging this pull request may close these issues.
4 participants
@ChrisRackauckas @ToucheSir @DhairyaLGandhi @CarloLucibello