-
Notifications
You must be signed in to change notification settings - Fork 266
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
Fixed a bunch of rename issues, including Charlie Zender's reported problem... #755
Fixed a bunch of rename issues, including Charlie Zender's reported problem... #755
Conversation
This pull request fixes 3 alerts - view on lgtm.com fixed alerts:
Comment posted by lgtm.com |
This pull request fixes 3 alerts - view on lgtm.com fixed alerts:
Comment posted by lgtm.com |
This pull request fixes 3 alerts - view on lgtm.com fixed alerts:
Comment posted by lgtm.com |
I think we need to hold off on this pr. |
@DennisHeimbigner do your merge first and I will adjust the PR... |
I'll bump this back down to |
Ed- what is the git clone argument for the |
[email protected]:NetCDF-World-Domination-Council/netcdf-c.git
But you can also just wait.
This PR contains a lot of changes all over the code, because as I wandered
through the code, I made fixes, added tests, and added documentation. I
also took out lines of code that were unnecessary.
So probably the best approach is for Ward to go ahead and make his merges.
Then I will prepare a new PR with just the rename fixes, the
string/fillvalue fixes, and all the extra tests (and maybe the
documentation). This should not cause you much trouble.
Then after your merge, I can swing back around and get everything else that
is still outstanding in my PR.
…On Tue, Jan 2, 2018 at 2:05 PM, Dennis Heimbigner ***@***.***> wrote:
Ed- what is the git clone argument for the
NetCDF-World-Domination-Council
repository. I cannot locate it on github.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#755 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEUr3Dw6dGC3an6_oPZugD6IPX6jjtJoks5tGpozgaJpZM4RQDpU>
.
|
@DennisHeimbigner @edhartnett Is your (Dennis) branch ready to be merged? |
Apparently I do not have read access to that repo. |
@DennisHeimbigner it is a public repo: You can't see the code from there? |
I got it; it was a ssh key problem. |
This pull request fixes 3 alerts - view on lgtm.com fixed alerts:
Comment posted by lgtm.com |
I'm going to take this PR down and let you guys catch up a little bit. When 4.6.0 is closer to release I will see if I can isolate some of my changes in a way that don't disrupt Dennis' work. I will keep an eye on the repo and jump back in sometime later this week or next week, when you have done some merges. |
Remove test code that uses the version 2 API if the version 2 API code is not being activated in the build. This was originally proposed in Unidata#656 with a better fix in Unidata#755 which was removed.
This PR is the result of additional testing for nc4var.c to increase code coverage in the tests.
It includes a fix for Charlie Zender's rename issue (#597), as well as another important rename issue.
It also include some less-critical fixes which prevent segfaults for strings and fill values.
Also much more testing and internal documentation. @WardF I suspect you might want to include this in 4.6.0 due to the fix for the rename issues. If so, I suggest you do the other PRs first and let me adjust this one against any merge issues as you proceed. Then when you are ready to merge it, there will be no conflicts.
Fixes #758.
Fixes #757.
Fixes #750.
Fixes #749.
Fixes #746.
Fixes #743.
Fixes #741.
Fixes #738.
Fixes #736.
Fixes #732.
Fixes #727.
Part of #702.
Part of #597.