Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataArray.from_dict: convert docstring code to doctest #6136

Closed
code-review-doctor opened this issue Jan 4, 2022 · 6 comments · Fixed by #6302
Closed

DataArray.from_dict: convert docstring code to doctest #6136

code-review-doctor opened this issue Jan 4, 2022 · 6 comments · Fixed by #6302

Comments

@code-review-doctor
Copy link
Contributor

What happened?

This test is passing in a str for dims:

d = {"dims": ("t")}
with pytest.raises(
ValueError, match=r"cannot convert dict without the key 'data'"
):
DataArray.from_dict(d)

dims should be a tuple. I suspect the intent was to specify d = {"dims": ("t",)} when the test was written.

This is small issue, but tests that call functions using the kind of data that would be used in "live" will improve quality of tests. Would be a shame for bug to affect users because tests use a str while in userspace a tuple is used.

What did you expect to happen?

code to be d = {"dims": ("t",)}

Minimal Complete Verifiable Example

No response

Relevant log output

No response

Anything else we need to know?

I'm happy to make a PR to fix this

Environment

NA

@code-review-doctor code-review-doctor added bug needs triage Issue that has not been reviewed by xarray team member labels Jan 4, 2022
@mathause
Copy link
Collaborator

mathause commented Jan 4, 2022

This tests checks if the appropriate error is raised if the passed dict misses the key "data". So it does IMHO not really matter what is passed as dims.

@keewis
Copy link
Collaborator

keewis commented Jan 4, 2022

this does indeed look like a typo (even though as @mathause says that value will not be checked because the absence of data is checked first) but in any case it is valid to pass a str: dims="t", dims=["t"] and dims=("t",) mean the same thing to the DataArray constructor for 1D data. Maybe we should just remove the parens to make this clear? I would have hoped that black takes care of something like that...

We also have the same issue in the docstring of DataArray.from_dict, which by the way would really benefit from converting the code block to a doctest-style example (it will fail right now).

@keewis keewis added topic-documentation topic-testing and removed bug needs triage Issue that has not been reviewed by xarray team member labels Jan 4, 2022
@code-review-doctor
Copy link
Contributor Author

I will make a PR to remove the paren to make the intent clearer 👍

Regarding black: worth also considering something like this to automatically highlight issues like this during PR :)

@mathause mathause changed the title [Bug]: Test not using "like live" data type for "dims" DataArray.from_dict: convert docstring code to doctest Jan 5, 2022
@mathause
Copy link
Collaborator

mathause commented Jan 5, 2022

I updated the title of the issue to reflect the new state after #6139.

@code-review-doctor
Copy link
Contributor Author

I have done an update to the docstring #6140

But note it is a minimal change rather than converting to doctest-style because I don't actually download or install xarray locally - I'm a bot that runs on AWS lambda to do static analysis checks on codebases for bugs. This repo was one of the 666 repos we run against when validating testing our new checkers.

As I do not have xarray setup locally I dont want to push larger code changes that have not been manually ran before I push, as there would be no confidence in the quality of the change :)

@keewis
Copy link
Collaborator

keewis commented Jan 5, 2022

okay, thanks, we can take it from here.

Note that we have a CI to automatically run doctests, so you could in theory push to verify changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants