rcsb / mmtf-cpp

The pure C++ implementation of the MMTF API, decoder and encoder.
MIT License
21 stars 24 forks source link

Add map template to checktype #30

Closed danpf closed 5 years ago

danpf commented 5 years ago

We wanted to decode into a map and this was required.

Do we 'need' checkType_ anyway? It's not particularly useful to me, wondering if anyone else uses it, or what its intent was originally!

danpf commented 5 years ago

The mac build is broken because travis is broken. https://travis-ci.community/t/issue-brew-formula-installation-fails-due-to-being-outdated/3872

the linux build is broken is because the default distribution changed from trusty to xenial.

danpf commented 5 years ago

Okay I set the default dist to be trusty now. But I think brew for travis is broken...

I suppose we could use conda instead of brew? I'm really not sure -- I don't have a mac....

frodofine commented 5 years ago

You're correct that the brew for Mac OSX is broken, this is has happened in other projects as well. Since GCC has long been unused for Mac OSX development I don't think it's important to test against it anymore. Otherwise, it can be fixed by adding the following to the .travis.yml file under the Mac OSX GCC test case. The addons: portion should be flush with osx_image.

      addons:
        homebrew:
          packages:
            - gcc@5
          update: true
danpf commented 5 years ago

great, thanks for the info! I was reading that adding update:true can add up to 30 minutes to the build time (for travis), so I'm more inclined to just remove it, however I will at least try it first

frodofine commented 5 years ago

In my experience that hasn't been the case, but it looks like it didn't work anyway.

danpf commented 5 years ago

okay I just removed it.

gtauriello commented 5 years ago

To answer the question on the "need" for checkType_: it was meant as a sanity check to ensure that the input has the types we expect in MMTF. Basically, I didn't want msgpack-conversions to silently convert data of input types that weren't supposed to be there in the first place. Now that we added extra properties, which can be fairly arbitrary, we could relax this.

I suggest the following: we keep the checks for the types used in the standard MMTF fields and add a generic checkType_ implementation that silently ignores all others?

Should be something along the lines of:

template <typename T>
void MapDecoder::checkType_(const std::string& key,
                            msgpack::type::object_type type,
                            const T& target) {
    // silently ignore targets with unknown type T
}

This should make sure that you don't need to write any checks in the future.

danpf commented 5 years ago

Okay this should be good. I removed the T* one as well since it could be ambiguous as to what type we would be trying to convert it to. (previously we cerred if it wasn't an array.

Also I'm pretty sure pointers require custom adapters.