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

Batch EJH Pull requests #844

Merged
merged 26 commits into from
Feb 7, 2018
Merged

Batch EJH Pull requests #844

merged 26 commits into from
Feb 7, 2018

Conversation

WardF
Copy link
Member

@WardF WardF commented Feb 6, 2018

Creating a pull request combining all outstanding ejh pull requests, in a branch with the .wif suffix. This will reduce the amount of CI tests which occur. The merge looks great, except for a few compile errors in Visual Studio. Addressing those and then will get things merged on in, clearing out the ejh backlog.

edhartnett and others added 24 commits January 30, 2018 09:35
@WardF WardF added this to the 4.6.1 milestone Feb 6, 2018
@WardF WardF self-assigned this Feb 6, 2018
@WardF
Copy link
Member Author

WardF commented Feb 6, 2018

@edhartnett So I'm able to duplicate the bug reported in #843 working with this group of combined pull requests, and have confirmed that the issue described is absent in the master branch. Before I separate this pull request back into the component parts, perhaps you have an idea off the top of your head which pull request this stems from? I can then remove it from this batch and merge everything but the problematic one, until we have a fix for it. Thanks for your help with this!

@edhartnett
Copy link
Contributor

Stand by, I will take a look and get back to you...

@edhartnett
Copy link
Contributor

@WardF I am building NCO so I can run this. Running into some problems I am working out with Charlie offline. I will come back when I have an answer.

@WardF
Copy link
Member Author

WardF commented Feb 6, 2018

Sounds good, if there are any broader netCDF issues that should be considered, you and @czender should feel free to pull myself and/or @dmh in. Thanks, I'll sit on this so that we're primed to merge once this is complete.

@WardF
Copy link
Member Author

WardF commented Feb 7, 2018

Ok, since the issue isn't related to your pull requests, I'll go ahead and merge all these shortly. Sorry for the confusion. I went back to the master version I tested on yesterday and it doesn't have this error but also says it's up to date, so I assume something is out of sync there. I blew the directory away and started with a fresh pull, now.

@edhartnett
Copy link
Contributor

Awesome.

Incidentally there is another LATEFILL issue open (not new behavior), one of the tests (tst_vars2.c) hits an error but instead of erroring out, just prints an error message. So it is ignored. But it is a LATEFILL error that (I believe) Quincey expected to occur, but apparently does not. I wrote an issue but have not otherwise touched it: #117.

According to @wkliao this should now be fixed: so the test should be changed to error out now instead of just printing an error message, and the #117 can be closed. I can do that as a separate PR or one of you guys can roll it in. That would be an extra test for correct behavior.

@WardF WardF merged commit f9b7f1d into master Feb 7, 2018
@WardF WardF deleted the ejh_batch.wif branch September 8, 2022 22:52
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.

nc_test/tst_open_cdf5.c does not include config.h
2 participants