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

Refactor packages to exclude from public namespace #1240

Closed
wants to merge 14 commits into from

Conversation

ambrosejcarr
Copy link
Member

@ambrosejcarr ambrosejcarr commented Apr 21, 2019

This PR has two main commits:

  • The first carries out the refactor, which is just a series of moves
  • The second appeases flake8, which reorders a bunch of imports.
  • Following commits address changes that had broken master related to documentation and are not relevant to the refactor.

The post-refactor namespace is a sub-set of the pre-factor namespace; no paths change, paths are only eliminated that are either duplicates, or which should not be exposed to users (e.g. starfish.multiprocessing)

It's also worth noting this was a clean refactor, I did not have to replace the packages whose imports are broken. For example, despite the fact that you can access starfish.multiprocessing from import starfish we, the developers, always use import starfish.multiprocessing. Thus, neither users nor developers expect these packages to be present.

Before clean-up:

image

After clean-up:

image

  • Potentially worth noting for follow-up PRs that we don't include ExpressionMatrix, DecodedSpots, or SegmentationMaskCollection in our default namespace.
  • Follow-ups could potentially simplify our suggested import strategy in our examples, it should now be possible to exclusively:
import starfish
from starfish import types

Question: if we want to continue importing types this way, should we also refactor starfish.types -> starfish._types?

@ambrosejcarr ambrosejcarr requested review from ttung, kne42, shanaxel42 and dganguli and removed request for kne42 April 21, 2019 15:40
@codecov-io
Copy link

codecov-io commented Apr 21, 2019

Codecov Report

Merging #1240 into master will increase coverage by 20.87%.
The diff coverage is 99.47%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1240       +/-   ##
===========================================
+ Coverage   67.41%   88.29%   +20.87%     
===========================================
  Files         132      132               
  Lines        4919     4919               
===========================================
+ Hits         3316     4343     +1027     
+ Misses       1603      576     -1027
Impacted Files Coverage Δ
starfish/_util/click/__init__.py 96.29% <ø> (ø)
starfish/_util/argparse.py 0% <ø> (ø)
starfish/_util/enum.py 100% <ø> (ø)
starfish/_intensity_table/concatenate.py 100% <ø> (ø)
starfish/_pipeline/__init__.py 100% <ø> (ø)
starfish/_codebook/_format.py 100% <ø> (ø)
starfish/_imagestack/parser/tileset/__init__.py 100% <ø> (ø)
starfish/_intensity_table/overlap.py 96.07% <ø> (ø)
starfish/_multiprocessing/shmem.py 100% <ø> (ø)
starfish/_imagestack/__init__.py 55.55% <ø> (ø)
... and 169 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2a7ccf...4eec0ea. Read the comment docs.

@ambrosejcarr ambrosejcarr requested a review from kne42 April 21, 2019 19:03
@ambrosejcarr ambrosejcarr force-pushed the ajc-namespace-cleanup branch from cb7a80f to a686b1d Compare April 22, 2019 00:17
…arfish into ajc-namespace-cleanup

* 'ajc-namespace-cleanup' of https://github.com/spacetx/starfish:
  more travis-only errors...
  fix error that only occurs on travis
  get docs building
  update makefile rules to ignore import ordering in top-level starfish __init__.py
  appease flake8
  refactor packages to exclude from public namespace
  Reorganize the presentation of the image filtering docs (#1229)
@ambrosejcarr ambrosejcarr force-pushed the ajc-namespace-cleanup branch from 998a741 to 718dc7e Compare April 22, 2019 12:56
@ambrosejcarr
Copy link
Member Author

Closing in favor of #1244

@ambrosejcarr ambrosejcarr deleted the ajc-namespace-cleanup branch April 23, 2019 14:28
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