Skip to content

Conversation

@tomvothecoder
Copy link
Collaborator

Description

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

If applicable:

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass with my changes (locally and CI/CD build)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

@tomvothecoder tomvothecoder self-assigned this Sep 24, 2025
@tomvothecoder tomvothecoder added the type: devops Testing, CI/CD, systems configuration label Sep 24, 2025
Copy link
Collaborator Author

@tomvothecoder tomvothecoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jasonb5 can you do a quick review of this PR? I highlighted the changes in my self-review below. The existing test build passes.

I want to get this in before the next release (next two weeks). Thanks.

@codecov
Copy link

codecov bot commented Sep 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (92b3d2e) to head (037e05a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #800   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines         1782      1784    +2     
=========================================
+ Hits          1782      1784    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@jasonb5 jasonb5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Just one thing, if None is False do we need to allow None for use_new_combine_kwargs_defaults?

- Format docstrings for readability
- Update test to remove `use_new_combine_kwarg_defaults` since `compat` and `join` are explicitly set
@tomvothecoder tomvothecoder force-pushed the devops/798-compat-join-behavior branch from 55506ed to 53ea7a6 Compare October 1, 2025 18:44
@tomvothecoder
Copy link
Collaborator Author

Thanks for the review @jasonb5.

LGTM Just one thing, if None is False do we need to allow None for use_new_combine_kwargs_defaults?

I actually removed use_new_combine_kwarg_defaults since I think it is a temporary placeholder (here 23a3eb2).

I realized that it is redundant to set compat/join explicitly while setting use_new_combine_kwargs_defaults. Both methods silence the FutureWarning.

@tomvothecoder tomvothecoder requested a review from Copilot October 1, 2025 18:46
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Preserve legacy Xarray merge/alignment behavior in xcdat.open_mfdataset by default, aligning with pre-Xarray-0.23 defaults.

  • Add compat and join keyword-only parameters with legacy defaults ("no_conflicts" and "outer") and forward them to xr.open_mfdataset
  • Update documentation to explain the preserved defaults and options
  • Adjust tests’ expectations to reflect legacy compat/join defaults

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
xcdat/dataset.py Adds compat and join parameters and forwards them via kwargs to xr.open_mfdataset; updates docstrings.
tests/test_dataset.py Updates expected merges in tests to use compat="no_conflicts" and join="outer".

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@tomvothecoder tomvothecoder requested a review from Copilot October 1, 2025 18:50
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@tomvothecoder tomvothecoder merged commit 17617e0 into main Oct 1, 2025
9 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in xCDAT Development Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: devops Testing, CI/CD, systems configuration

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[DevOps] Preserve Xarray's legacy compat and join behaviors and address FutureWarning

3 participants