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

RFC: public/private separation #1244

Merged
merged 1 commit into from
Apr 23, 2019
Merged

RFC: public/private separation #1244

merged 1 commit into from
Apr 23, 2019

Conversation

ttung
Copy link
Collaborator

@ttung ttung commented Apr 22, 2019

Some basic rules of thumb:

  1. starfish_impl code should never reference starfish.
  2. tests in starfish_impl should generally reference starfish whenever possible. exception: imports in tests for package X should use relative imports to its enclosing package whenever possible. for example, importing ImageStack from ImageStack tests should be done as from ..imagestack import ImageStack. Rationale: it's easy to move code around without breaking references.
  3. notebook/vignette code should never use starfish_impl.

I also created some public APIs where they did not previously exist. Basically, go check out what was added in starfish/

@ttung
Copy link
Collaborator Author

ttung commented Apr 22, 2019

Contents of starfish:

In [2]: dir(starfish)                                                                                                                                                                                                                                                     
Out[2]: 
['Codebook',
 'Experiment',
 'ExpressionMatrix',
 'FieldOfView',
 'ImageStack',
 'IntensityTable',
 '__builtins__',
 '__cached__',
 '__doc__',
 '__file__',
 '__is_release_tag__',
 '__loader__',
 '__name__',
 '__package__',
 '__path__',
 '__spec__',
 '__version__',
 'config',
 'display',
 'image',
 'spots',
 'starfish']

In [3]:

@ttung
Copy link
Collaborator Author

ttung commented Apr 22, 2019

starfish autocomplete:

In [2]: starfish. 
                  Codebook           display()          ExpressionMatrix   image              IntensityTable     starfish           starfish.wdl      
                  config             Experiment         FieldOfView        ImageStack         spots              starfish.egg-info/                                                                                                                                  

@ttung ttung force-pushed the tonytung-private-public branch from 7bef9c5 to abb1932 Compare April 22, 2019 18:27
@ambrosejcarr
Copy link
Member

I see that I dropped it too, but I think starfish.data would be good to drop into the main namespace.

Otherwise this LGTM from the end-user perspective.

@codecov-io
Copy link

codecov-io commented Apr 22, 2019

Codecov Report

Merging #1244 into master will decrease coverage by 2.38%.
The diff coverage is 97.2%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1244      +/-   ##
==========================================
- Coverage   88.86%   86.47%   -2.39%     
==========================================
  Files         140      146       +6     
  Lines        5244     5524     +280     
==========================================
+ Hits         4660     4777     +117     
- Misses        584      747     +163
Impacted Files Coverage Δ
starfish/core/spacetx_format/test_field_of_view.py 100% <ø> (ø)
starfish/core/spacetx_format/test_coordinates.py 100% <ø> (ø)
starfish/core/types/_constants.py 100% <ø> (ø)
starfish/core/spots/__init__.py 100% <ø> (ø)
starfish/core/_version.py 41.36% <ø> (ø)
starfish/core/util/try_import.py 50% <ø> (ø)
starfish/core/errors.py 100% <ø> (ø)
starfish/core/spacetx_format/test_fov_manifest.py 100% <ø> (ø)
starfish/core/recipe/__init__.py 100% <ø> (ø)
starfish/core/recipe/errors.py 100% <ø> (ø)
... and 138 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 efa9d78...99718e8. Read the comment docs.

@ttung ttung force-pushed the tonytung-private-public branch 4 times, most recently from 1f98d98 to 528b575 Compare April 22, 2019 20:21
@ttung
Copy link
Collaborator Author

ttung commented Apr 22, 2019

I see that I dropped it too, but I think starfish.data would be good to drop into the main namespace.

I didn't move starfish.data...?

@ambrosejcarr
Copy link
Member

I mean: it should be imported by default.

@ttung
Copy link
Collaborator Author

ttung commented Apr 22, 2019

Was it done so previously?

@ttung ttung force-pushed the tonytung-private-public branch 8 times, most recently from fb6f366 to aa84692 Compare April 23, 2019 07:39
Some basic rules of thumb:

1. `starfish.core` code should never reference `starfish`.
2. tests in `starfish.core` should generally reference `starfish` whenever possible.  exception: imports in tests for package X should use relative imports to its enclosing package whenever possible.  for example, importing `ImageStack` from `ImageStack` tests should be done as `from ..imagestack import ImageStack`.  Rationale: it's easy to move code around without breaking references.
3. notebook/vignette code should never use `starfish.core`.

I also created some public APIs where they did not previously exist.  Basically, go check out what was added in `starfish/`
@neuromusic
Copy link
Collaborator

I want to let @ambrosejcarr or @shanaxel42 do the actual PR review, but I really dig the API changes here

@ttung
Copy link
Collaborator Author

ttung commented Apr 23, 2019

By the way, this is passing all notebooks now.

Copy link
Collaborator

@shanaxel42 shanaxel42 left a comment

Choose a reason for hiding this comment

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

fiiiiiiine

@ttung ttung merged commit a3990d2 into master Apr 23, 2019
@ttung ttung deleted the tonytung-private-public branch April 23, 2019 16:06
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.

5 participants