issue_comments

keeping the full list in memory isn't expensive at all IIUC

I agree, this should not be an issue at all: we're iterating over datasets multiple times so we definitely need it to be a sequence (currently it's some kind of iterator object, e.g. the one returned by dict.keys).

Will it not cripple performance due to the non-lazy evaluation of the list ?

I don't think so, but I'm not sure — keeping the full list in memory isn't expensive at all IIUC. And either way, it'll confirm the reason for the test failures for now.

I also started the test for the concat function, I discovered that if the first element determine between _dataarray_concat and _dataset_concat. I am not sure if I need to do several commits (type checking dataset, type checking data array) and several test.

Here is the draft for the tests:

Those look good! Feel free to add them directly and we can discuss them there, and see the test results. Thanks!

Will it not cripple performance due to the non-lazy evaluation of the list ?

I also started the test for the concat function, I discovered that if the first element determine between _dataarray_concat and _dataset_concat. I am not sure if I need to do several commits (type checking dataset, type checking data array) and several test.

Here is the draft for the tests: ``` def test_concat_check_input_type(): ds = Dataset({"foo": 1}, {"bar": 2}) da = Dataset({"foo": 3}, {"bar": 4}).to_array(dim='foo')

# concatenate a list of non-homogeneous types must raise TypeError
with pytest.raises(TypeError, match="Some elements in the input list datasets are not 'DataSet'"):
    concat([ds, da], dim="foo")

# concatenate a list of non-homogeneous types must raise TypeError
with pytest.raises(TypeError, match="Some elements in the input list datasets are not 'DataArray'"):
    concat([da, ds], dim="foo")

```

Code I plan to add also for the typing check in _dataarray_concat: from .dataarray import DataArray arrays = list(arrays) if not all(isinstance(array, DataArray) for array in list(arrays)): raise TypeError("Some elements in the input list datasets are not 'DataArray'") (I need to add from .dataarray import DataArray to be able to use the type)

Ok! I'll do that in the next few days :) I'm trying to get more familiar with the whole procedure

Hi @thomashirtz ! Thanks for the PR.

The code looks reasonable, though it's generating test failures. To prioritize giving quick feedback over complete feedback — possibly datasets is sometimes a generator. We could add datasets = list(datasets) above the current code. If that doesn't fix it, feel free to take an error and try debugging what's going on, or someone can help.

We should add a test for this too.