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

Make submodules private #28

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Conversation

gmgunter
Copy link
Member

Prefix submodule names with an underscore to indicate that they're not intended to be accessed by downstream projects.

Only symbols exposed in the top-level namespace should be considered public. We don't want to treat the locations of symbols in individual submodules as part of the API. Instead, we'd prefer to be able to reorganize things in the future without causing breaking changes.

In the future, we might add public submodules (such as a tophu.typing submodule for custom type annotations) to store public symbols that don't belong in the top-level namespace. But, for now, every symbol that's intended to be public should be importable directly from tophu.

Prefix submodule names with an underscore to indicate that they're not
intended to be accessed by downstream projects.

Only symbols exposed in the top-level namespace should be considered
public. I don't want to treat the locations of symbols in individual
submodules as part of the API -- I'd prefer to be able to reorganize
things in the future without causing breaking changes.

In the future, we might add public submodules (such as a `tophu.typing`
submodule for custom type annotations) to store public symbols that
don't belong in the top-level namespace. But, for now, every symbol
that's intended to be public should be importable directly from `tophu`.
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #28 (c8d0047) into main (484407c) will decrease coverage by 0.11%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
- Coverage   97.55%   97.45%   -0.11%     
==========================================
  Files           8        8              
  Lines         696      668      -28     
==========================================
- Hits          679      651      -28     
  Misses         17       17              
Files Changed Coverage Δ
src/tophu/_filter.py 98.14% <ø> (ø)
src/tophu/_unwrap.py 98.61% <ø> (ø)
src/tophu/_util.py 100.00% <ø> (ø)
src/tophu/_multiscale.py 95.16% <94.11%> (ø)
src/tophu/__init__.py 100.00% <100.00%> (ø)
src/tophu/_io.py 96.68% <100.00%> (ø)
src/tophu/_multilook.py 100.00% <100.00%> (ø)
src/tophu/_upsample.py 97.56% <100.00%> (ø)

@scottstanie
Copy link
Contributor

any chance you could make coarse_unwrap exposed too? or do you think it's possible to get the same result with multiscale_unwrap?

(i found it useful enough that I stuck it in as a temporary easter egg while tophu was being remodeled
https://github.com/opera-adt/dolphin/blob/main/src/dolphin/unwrap.py#L424 )

@gmgunter
Copy link
Member Author

any chance you could make coarse_unwrap exposed too?

Yeah, it's on my list. I have an old half-finished branch somewhere with the necessary changes. Just need to clean it up and add a unit test. Thanks for the heads up.

@gmgunter gmgunter merged commit 1cb3a47 into isce-framework:main Aug 24, 2023
6 checks passed
@gmgunter gmgunter deleted the private-submodules branch August 24, 2023 16:02
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.

2 participants