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

Rename accessor classes and methods for API consistency #142

Merged
merged 3 commits into from
Nov 9, 2021

Conversation

tomvothecoder
Copy link
Collaborator

@tomvothecoder tomvothecoder commented Nov 9, 2021

Description

The goal of this PR is to make the API naming convention consistent across accessor classes.

The Dataset portion of accessor class names are removed because it is redundant.

  • Closes Align xcdat.XCDATAccessor public API names with other accessor classes #140
  • Rename DatasetBoundsAccessor to BoundsAccessor (breaking change)
  • Rename DatasetSpatialAverageAccessor to SpatialAverageAccessor (breaking change)
  • Rename SpatialAverageAccessor.avg() to SpatialAverageAccessor.spatial_avg() (breaking change)
    • The original reason why it was called .avg() was because the accessor class and decorator (ds.spatial.avg) already describes what the averaging operation was targeting. However, it was not aligned with ds.xcdat.spatial_avg(), which can throw users off.
  • Add tests for SpatialAverageAccessor class
  • Add accessor classes to xcdat.__init__.py
  • Remove flake8 noqa comments in xcdat.py

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) -- won't increment as a major release, just note in v0.2.0 changelog

- Rename `DatasetBoundsAccessor` to `BoundsAccessor`
- Rename `DatasetSpatialAverageAccessor` to `SpatialAverageAccessor`
- Rename `SpatialAverageAccessor.avg()` to `SpatialAverageAccessor.spatial_avg()`
- Add tests for `SpatialAverageAccessor` class
@tomvothecoder tomvothecoder added type: enhancement New enhancement request Priority: High breaking-change A breaking change, intended for a major release. labels Nov 9, 2021
@tomvothecoder tomvothecoder self-assigned this Nov 9, 2021
@tomvothecoder
Copy link
Collaborator Author

Hi @pochedls, it'd be great if you can review this PR so you're aware of the changes since it affects the spatial averaging API. Thanks!

- Remove flake8 noqa comments in `xcdat.py`
Copy link
Collaborator

@pochedls pochedls left a comment

Choose a reason for hiding this comment

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

No objections to the API name changes (I think this is a good idea).

@lee1043
Copy link
Collaborator

lee1043 commented Nov 9, 2021

I second @pochedls!

@tomvothecoder
Copy link
Collaborator Author

Awesome, thanks @pochedls and @lee1043

@tomvothecoder tomvothecoder merged commit af67830 into main Nov 9, 2021
@tomvothecoder tomvothecoder deleted the feature/140-method-name-alignment branch November 9, 2021 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A breaking change, intended for a major release. type: enhancement New enhancement request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align xcdat.XCDATAccessor public API names with other accessor classes
3 participants