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

a clean commit for #383 #731

Merged
merged 8 commits into from
Feb 2, 2018
Merged

a clean commit for #383 #731

merged 8 commits into from
Feb 2, 2018

Conversation

wkliao
Copy link
Contributor

@wkliao wkliao commented Dec 21, 2017

This PR contains only my changes for #383.

@WardF
Copy link
Member

WardF commented Dec 21, 2017

Looks good I will see how quickly I can get this in, we are trying to get 4.6.0 (previously 4.5.1, but with compression plugin support we have bumped to 4.6.0) out ASAP. I see the checks failed, will investigate in AM.

@@ -9,7 +9,9 @@
#ifndef _DISPATCH_H
#define _DISPATCH_H

#if HAVE_CONFIG_H
Copy link
Contributor

Choose a reason for hiding this comment

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

We always have config.h, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a common practice when using autotools.

@edhartnett
Copy link
Contributor

Awesome, thanks. Much better than the other PR! ;-)

@WardF if you are going to include this in 3.6.0 then I think we will need a little time after this merge is complete to look into all the other reported fill value issues and make sure all is working as it should. For example I have some new tests that will need to be modified to check per-variable fill mode instead of per-file fill mode for classic formats.

@edhartnett
Copy link
Contributor

@wkliao after this PR is merged, what happens when the user calls nc_set_fill() to try and set fill value for the whole file. Does that still work?

@@ -158,6 +158,9 @@ NCD2_rename_var(int ncid, int varid, const char *name);
extern int
NCD2_var_par_access(int, int, int);

extern int
NCD2_def_var_fill(int, int, int, const void *);

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, why was this moved? Seems like at least three files are changing for no real reason. Or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, never mind. I see that you are moving things outside the #ifdef USE_NETCDF4. Now I get it. ;-)

@edhartnett
Copy link
Contributor

OK, to answer my own question, after this PR, when nc_set_fill() is called it will correctly set the fill value of all variables.

@WardF
Copy link
Member

WardF commented Dec 21, 2017

I am reviewing pull requests now and trying to prioritize them, I'd like to include this but we shall see, I also need to get 4.6.0 out to revert some behavior that was introduced in 4.5.0. So.. maybe. :)

@WardF WardF self-assigned this Jan 2, 2018
@WardF WardF added this to the 4.6.0 milestone Jan 2, 2018
@WardF WardF modified the milestones: 4.6.0, 4.6.1 Jan 25, 2018
@WardF WardF merged commit 8c6defe into Unidata:master Feb 2, 2018
@WardF
Copy link
Member

WardF commented Feb 2, 2018

Thanks!

@wkliao wkliao deleted the nc3-per-var-fill-v2 branch September 15, 2018 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants