Use explicit type check in as_compatible_data instead of blanket access to values attribute

[blsqr]

This change was prompted by the fact that getattr(data, 'values', data)
affected any kind of data with a values attribute, which is not the
desired behaviour for core.variable.as_compatible_data.

This also extends tests to assert that custom objects with such an
attribute are not attempted to be converted.

I'm happy to write additional tests, if you could briefly sketch further requirements.

• Implemented the changes
• Closes Variable.__setitem__ coercing types on objects with a values property #2097
• Tests added
  • Sufficient?
• Fully documented, including whats-new.rst for all changes and api.rst for new API

[pep8speaks]

Hello @blsqr! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-12-01 15:58:23 UTC

[dcherian]

Thanks @blsqr. The tests are failing at this line:
orig = Variable(dims=('x'), data=array, attrs={'foo': 'bar'})

so your change isn't working yet. Can you fix it?

[blsqr]

@dcherian The most recent pipeline now fails in the TestRasterio.test_rasterio_vrt test. This seems unrelated... but I'm not sure if they might be connected.

Could you give me a hint on how to continue? Or need this be fixed elsewhere (or is already fixed) and I need to update this topic branch?

[dcherian]

We can ignore that for now.

[dcherian]

I'm not sure that Variable is wrapping this CustomValues thing properly though. Someone else will need to chime in.

[keewis]

@blsqr, I'm sorry for taking so long to review this. I updated your PR so this should be ready for merging.

I'm not sure that Variable is wrapping this CustomValues thing properly though.

that turned out not to be true, we can't wrap it in a list because then as_compatible_data wouldn't be able to find .values. Instead, Variable(dims=(), data=array) (or converting it to a duckarray) is the way to go.

[blsqr]

@keewis Thanks for looking into this and updating the PR! :)

[keewis]

thanks again, @blsqr