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

Exporting only long-form boundary condition constructors? #1140

Closed
ali-ramadhan opened this issue Nov 4, 2020 · 1 comment · Fixed by #1489
Closed

Exporting only long-form boundary condition constructors? #1140

ali-ramadhan opened this issue Nov 4, 2020 · 1 comment · Fixed by #1489

Comments

@ali-ramadhan
Copy link
Member

ali-ramadhan commented Nov 4, 2020

Currently the Oceananigans module exports

Flux, Value, Gradient, NormalFlow,
FluxBoundaryCondition, ValueBoundaryCondition, GradientBoundaryCondition,

so there's some redundancy (perhaps we should export one set to reduce namespace pollution) but a minor practical issue is that the Flux type conflicts with the popular Flux.jl package (I guess I'm the only one using them together right now but there may be more in the future?). There's also some inconsistency in exporting NormalFlow but not NormalFlowBoundaryCondition.

In deciding on what to export for the user interface (see #1132) I'm wondering what do people think about only exporting the long-name version, e.g. FluxBoundaryCondition instead of Flux.

I think this will have a few benefits:

  1. Lower probability for conflicts. Flux is one example, but Value and Gradient are pretty generic terms so I wouldn't be surprised if they conflict with exports from other packages future users may want to work with.
  2. Scripts might read more intuitively, e.g. because you say "a flux boundary condition" and not "a boundary condition of type flux".
  3. If we decide to export the complete set of boundary conditions we could do it by exporting PeriodicBoundaryCondition without having to worry about conflicting with the Periodic topology.

X-Ref: #1132

@glwagner
Copy link
Member

glwagner commented Nov 4, 2020

I'm fine with this. Also helps with #1119 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants