bluesky / suitcase-core

data export facilities for NSLS-II
https://blueskyproject.io/suitcase
Other
2 stars 13 forks source link

ENH: Add spec to documents and documents to spec #11

Closed ericdill closed 8 years ago

ericdill commented 8 years ago

To go in after #8

danielballan commented 8 years ago

I would have made those print statements warnings so that they are easier to test and easier to silence if users so choose. But I'm OK with print statements.

ericdill commented 8 years ago

Ah good point. Thanks. All of the document conversion code was copied out of a jupyter notebook so pointers like that are very welcome!

ericdill commented 8 years ago

@danielballan so just this?

import warnings
warnings.warn(msg)
danielballan commented 8 years ago

Yes.

ericdill commented 8 years ago

Still TODO:

ericdill commented 8 years ago

Tests cannot be run on py2.7 on travis because this PR adds a bluesky dep for CallbackBase. I can either:

  1. Make this repo py3 only
  2. Break out callbacks from bluesky so that I can keep this repo py2 and py3
tacaswell commented 8 years ago

You can also do something like

try:
    import module_that_needs_bluesky
except ImportError:
    module_that_needs_bluesky = None

def test():
    # make this a decorator
    if module_that_needs_bluesky is None:
        raise SkipTest

and drop bluesky as a required dependancy of suitcase.

ericdill commented 8 years ago

Well the main feature of this PR doesn't even work if I drop bluesky as a required dep...

ericdill commented 8 years ago

I guess my question was really asking if it was important to maintain python 2 compatibility on suitcase

tacaswell commented 8 years ago

You can still install bluesky for python3 only on travis to test this feature. I do not see a problem with having some parts of the library being python 3 only and others python2/3.

Long term we do want to pull all of the callbacks out of bluesky, I am just not sure now is the right time to do that (we need to feature freeze next week). I think this sort of hack is the fastest way to get this merged so we can deploy it for next cycle.

ericdill commented 8 years ago

Ok that makes sense. Thanks. Will update now.

ericdill commented 8 years ago

Actually what I'm going to do is just copy/paste the CallbackBase code from bluesky to suitcase.spec and then just remove the bluesky dep entirely. That seems simpler than hacking around in the .travis.yml, spec.py and tests/test_spec.py

tacaswell commented 8 years ago

that also makes sense.

codecov-io commented 8 years ago

Current coverage is 94.45%

Merging #11 into master will increase coverage by +0.28%

@@           master        #11   diff @@
========================================
  Files           2          4     +2   
  Lines         103        595   +492   
  Methods         0          0          
  Branches        0          0          
========================================
+ Hits           97        562   +465   
- Misses          6         33    +27   
  Partials        0          0          

Powered by Codecov. Last updated by 3474fd1...218ae3e

ericdill commented 8 years ago

This is ready for review. It is long, but ~4k lines are just the specfile. This PR adds the capability to round trip document schema <-> specfile with regards to ascan/Scan and dscan/RelativeScan. attn @tacaswell @danielballan

danielballan commented 8 years ago

Wow. A+. A very clean implementation of a monumentally messy problem. Take or leave my one suggestion.

ericdill commented 8 years ago

Wow. A+. A very clean implementation of a monumentally messy problem.

Thanks for the kind words. This was a pile of work, so it's nice to get some very positive feedback :sweat_smile:

klauer commented 8 years ago

Agreed with @danielballan - great work. I don't really see anything to even nitpick.

This will get a lot of use, I'm sure.