issue_comments

I would like to continue to contribute to xarray, unfortunately I have many important matters theses days. I may contribute again from end of summer. If someone wants to tackle this issue until then, he can

Ok, I think I understand better now Almost finished, I just have one test that fails currently, it is this one: https://github.com/pydata/xarray/blob/e17cf595c84dffdd73c668f6d945c1b0eeba13d6/xarray/tests/test_dataset.py#L3299-L3306

The error changed from a KeyError to the error that I wrote in the constructor : ```

          raise TypeError(f'The dimension provided is a tuple, you may intended to pass a list')

E TypeError: The dimension provided is a tuple, you may intended to pass a list ```

I am not sure what I should do with it, should I delete it ?

(Thanks for your help :))

I think I am quite confused by the fact that 'one' dimension can be a tuple (also I couldn't find a way to successfully create a dataset with a dim being a tuple, even by tweaking your example using list)

Was it what you imagined for the dataset construction ? (If I understand right, a virtual variable can't be a tuple) ``` def _construct_dataarray(self, name: Hashable) -> "DataArray": """Construct a DataArray by indexing this dataset""" from .dataarray import DataArray

    try:
        variable = self._variables[name]
    except KeyError:
        if isinstance(name, tuple):
            raise TypeError(f'The dimension "{name}" provided is a tuple, you may be intended to pass it as a list')
        else:
        _, name, variable = _get_virtual_variable(
            self._variables, name, self._level_coords, self.dims
        )

```

You seemed to all agree on the solution 1:

Explicitly allow tuples as dimension and variable names. This is probably the most mypy-compatible, since we use Hashable in lots of places

If you allow explicitly tuple, doesn't it mean I need to edit this part (as mentioned by @mathause) : if not isinstance(variables, (list, tuple)): variables = [variables] (I tried and it works well)

But you said after :

If it's a tuple, that message can suggest passing it as a list.

(I find it a little bit weird to give dimensions in list, no ? I thought that generally tuples were more adapted to provide dims)

Can you tell me which test-case I can implement to fulfill all the requirements needed ? I can't think of test cases that I can implement except the one suggested first, and that one passes with the first modification

Also the code you mentioned is for one dim, but in this case we can have a list of dims, it does means that I need to do a 'for loop' for checking the existence of the dims, right ?

(@max-sixty , I originally took this one because I wanted to do one more ~easy PR. I just saw you modified the tags, lmk if this one is problematic or if you know other issues I could tackle)

@mesejo Do you still work on it ? If not, I am interested to work on it

Yes I understand now I committed myself to do the last minor adjustment on github, I really should not have facepalm (pre-commit is great)

Ha, no problem. And don't worry about the commit quality — it's all squashed into one at the end. I generally write one good message at the start but that's all.

Good that they are going to be squashed, because this PR starts to look very much chaotic ah ah
So when they are squashed, the message of the first commit get automatically picked ?

One small change — could we say in the error message that they all need to be either all DataArrays or all Datasets? For a new user, the currently proposed messages could suggest that they all need to be the type in the message, and that type is determined by the order of the args, which is a bit confusing. That might mean the error message is the same for both, or only slightly different. Does that make sense?

It does make sense
Done ✔

I also edited whats new

Oh, I just found, I wanted to do three separate commits for each changes, but when I did the dataarray commit, I somehow reverted the changes of the dataset commit facepalm. All the tests were working on my computer because I still had the changes locally.

Ok I'll do the whatsnew soon :) I am a little bit stuck though, I don't really found why my test would cause a ValueError in those tests

This looks great! Do you want to add a note to the whatsnew?

What is the whatsnew ? I am still checking why there is some test failing 😶

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

@max-sixty I did my first pull request here, let me know if it is alright :)

I almost never contributed to someone else's project before on github, however I watched several tutorials.

It does make sense! I'll let you know in this thread if I really can't figure a step Thanks :)

I would like to start contributing to the xarray library (it would be my first contribution), I figure I can start by trying to make this error message. But I am not sure what is the best error message/best implementation. If anyone have some advise for this specific problem, it would be very welcome :) (ping @dcherian because of our discussion)