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.

Sign up for GitHub

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

Jump to bottom

move da and ds fixtures to conftest.py #6730

Merged
merged 7 commits into from
Jul 11, 2022
Merged

move da and ds fixtures to conftest.py #6730

merged 7 commits into from
Jul 11, 2022

Conversation

mathause
Copy link
Collaborator

@mathause mathause commented Jun 27, 2022 •
edited
Loading

This PR renames the da and ds fixtures (to da_fixture and ds_fixture) and moves them to conftest.py. This allows to remove the flake8 error suppression for the tests and seems more how the fixtures are intended to be used (from the pytest side). I think the name changes makes it a bit more obvious what happens but moving them to may make it a bit less obvious (if you don't know where to look).

Removing the flake8 error ignores also unearthed some unused imports:

xarray/setup.cfg

Lines 155 to 156 in 787a96c

per-file-ignores =
xarray/tests/*.py:F401,F811

(What I actually wanted to do is move the tests for rolling to it's own file - but I think it makes sense to do this first.)

@max-sixty
Copy link
Collaborator

I agree with moving them!

But I haven't seen ..._fixture in pytest before, and it makes the names longer — could I ask what the benefit of that is? If we wanted dataarray & dataset then that would be fine...

@mathause
Copy link
Collaborator Author

I found the names da and ds a bit ambiguous. I am happy to adapt to something more common of course - your suggestion would be to name them dataarray and dataset?

@max-sixty
Copy link
Collaborator

your suggestion would be to name them dataarray and dataset?

That would be great I think!

@headtr1ck
Copy link
Collaborator

headtr1ck commented Jun 30, 2022 •
edited
Loading

Any chance that these might shadow the dataarray and dataset modules?
Although I think it is not very common to import these modules directly in the tests.

@mathause
Copy link
Collaborator Author

mathause commented Jul 1, 2022

Any chance that these might shadow the dataarray and dataset modules? Although I think it is not very common to import these modules directly in the tests.

Good point, haven't thought of this. But I think these are not imported. Staying with da and ds would also reduce the diff...

@max-sixty
Copy link
Collaborator

Yes, agree they are not imported — good point but I think this would be OK

@mathause
Copy link
Collaborator Author

mathause commented Jul 1, 2022

@max-sixty: so keep da or switch to dataarray for the fixture?

@max-sixty
Copy link
Collaborator

Don't put such decisions on a mere mortal!

I would say switch given it sounds like da & ds caused some confusion. Even if personally I was OK with da & ds

@headtr1ck headtr1ck mentioned this pull request Jul 2, 2022
Merged
4 tasks
@mathause
Copy link
Collaborator Author

I renamed the fixtures back to da & ds to make the PR smaller & will merge once the checks are green...

@mathause mathause changed the title rename and move da and ds fixtures move da and ds fixtures to conftest.py Jul 11, 2022
@mathause mathause enabled auto-merge (squash) July 11, 2022 12:26
@mathause mathause merged commit 0dedcb0 into pydata:main Jul 11, 2022
dcherian added a commit to keewis/xarray that referenced this pull request Jul 22, 2022
@dcherian
Merge branch 'main' into duckarray-tests
21a5838 
* main: (313 commits)
  Update whats-new
  Release notes for v2022.06.0 (pydata#6815)
  Drop multi-indexes when assigning to a multi-indexed variable (pydata#6798)
  Support NumPy array API (experimental) (pydata#6804)
  Add cumsum to DatasetGroupBy (pydata#6525)
  Refactor groupby binary ops code. (pydata#6789)
  Update DataArray.rename + docu (pydata#6665)
  Switch to T_DataArray and T_Dataset in concat (pydata#6784)
  Fix typos found by codespell (pydata#6794)
  Update groupby attrs tests (pydata#6787)
  Update map_blocks to use chunksizes property. (pydata#6776)
  Fix `DataArrayRolling.__iter__` with `center=True` (pydata#6744)
  [test-upstream] Update flox repo URL (pydata#6780)
  Move _infer_meta_data and _parse_size to utils (pydata#6779)
  Make the `sel` error more descriptive when `method` is unset (pydata#6774)
  Move Rolling tests to their own testing module (pydata#6777)
  [pre-commit.ci] pre-commit autoupdate (pydata#6773)
  move da and ds fixtures to conftest.py (pydata#6730)
  Bump EnricoMi/publish-unit-test-result-action from 1 to 2 (pydata#6770)
  Type shape methods (pydata#6767)
  ...
@Illviljan Illviljan mentioned this pull request Nov 5, 2022
Open
@mathause mathause deleted the rename_da_ds_fixture branch December 5, 2022 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcherian dcherian dcherian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mathause @max-sixty @headtr1ck @dcherian