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

BUG Fix leftover references to ETA_STR #126

Merged
merged 3 commits into from Jan 26, 2017

Conversation

spencerkclark
Copy link
Collaborator

Some references to ETA_STR were unintentionally not converted to internal_names.ETA_STR as part of #90. This short PR fixes this bug.

@spencerahill spencerahill merged commit 741569b into spencerahill:develop Jan 26, 2017
@spencerahill
Copy link
Owner

Good catch. Thanks!

@spencerahill
Copy link
Owner

This is small enough that we can skip it, but let's remember moving forward to add What's New entries for all new bug fixes and features.

@spencerkclark spencerkclark deleted the fix-ETA-STR branch January 26, 2017 23:13
@spencerkclark spencerkclark mentioned this pull request Jan 26, 2017
7 tasks
@spencerkclark
Copy link
Collaborator Author

@spencerahill a quick and dirty unit test for this might be to include some three-dimensional test data in our test/data/netcdf/*.nc files, and compute a vertically integrated quantity (say column integrated water vapor for example) in test_calc_basic.py. It's not obvious to me how to pin this down to a test of a single method in calc.py.

@spencerkclark
Copy link
Collaborator Author

I fully agree that we need to have at least some Travis coverage here, and not rely on us simply running things ourselves every time we update the code. @spencerahill what are your thoughts? Does my suggestion above seem reasonable?

@spencerahill
Copy link
Owner

@spencerkclark that seems like a good approach, although make sure that it's only a single timestep for package size reasons.

It's not obvious to me how to pin this down to a test of a single method in calc.py.

That's what I was thinking also.

@spencerahill
Copy link
Owner

I opened #129 to track this further

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

Successfully merging this pull request may close these issues.

None yet

2 participants