Repository: ioos/compliance-checker
Pull request #772 (2020-03-17 19:30:59)

Improve ioos_ingest check
Any value supplied other than "true" or "false" should fail;
add improved error message for clarity.

Now, no more "ioos_ingest" message when testing on https://pae-paha.pacioos.hawaii.edu/erddap/tabledap/WQB-04.html:

```
$ dalton@ubuntu:~/compliance-checker$ python cchecker.py -t ioos:1.2 WQB-04.ncCF | grep "ioos_ingest"
Running Compliance Checker on the datasets from: ['WQB-04.ncCF']
```

```

mwengren: I don't know, my feeling is it should be case insensitive, but it should match rules for `gts_ingest`.  I don't think we specify in the guidance one way or the other.

Is there any other precedent?
Bobfrat: I tend to agree but this attribute is specifically for NDBC right? Do we know if they are expecting it to be case sensitive?
daltonkell: There are plenty of scenarios where we look for case-insensitive values, e.g.

https://github.com/ioos/compliance-checker/blob/5ff9f248f38f17d17f08d9787fc1fa4b240bea34/compliance_checker/cf/cf.py#L3804-L3805

These are camel-case in the spec
mwengren: Let's use the same case-sensitivity rules for both `ioos_ingest` and `gts_ingest` whichever way we go.  

@kbailey-noaa @jessicaaustin: do either of you happen to recall if it matters to NDBC case of `gts_ingest` flag?

Unless they've told us otherwise, I think we should have the checker allow either.   

In terms of precedent, I was more wondering what's out there in the wild in terms of `True|true` and `False|false` string attribution customs in netCDF files.  
benjwadams: I can't merge this until I know the desired behavior WRT case-sensitivity.
Bobfrat: that and the merge conflict...
jessicaaustin: I would follow whatever the convention is for the rest of the attributes. If typically things are case-insensitive, then these can be too. 

We have no idea if NDBC checks are case-insensitive or not. But [ERDDAP search definitely doesn't care](http://erddap.cencoos.org/erddap/search/index.html?page=1&itemsPerPage=1000&searchFor=GTS_INGEST%3Dtrue).
daltonkell: Following with other conventions, I'll make this case-insensitive.
mwengren: I just emailed NDBC to make sure they were aware.   Not expecting any issue with that so case-insensitive is the way to go, thanks.  
daltonkell: @benjwadams Feedback implemented.

> In fact you could just do if isinstance(igst, str) and igst.lower() not in ("true", "false"): since the not None implicitly is also done by the isinstance check.

It still must check if the value is anything *other* than None, though, because the value can *not* be supplied and still pass.

Patch:
diff --git a/compliance_checker/ioos.py b/compliance_checker/ioos.py
index 8da9206..16c310b 100644
--- a/compliance_checker/ioos.py
+++ b/compliance_checker/ioos.py
@@ -467,6 +467,8 @@ class IOOS1_2Check(IOOSNCCheck):
             #'creator_type',
             'institution',
             'instrument',
+            # checked in check_ioos_ingest
+            #'ioos_ingest',
             'keywords',
             ('platform_id', IOOS1_2_PlatformIDValidator()), # alphanumeric only
             'publisher_address',
@@ -548,7 +550,7 @@ class IOOS1_2Check(IOOSNCCheck):
         If a dataset contains the global attribute ioos_ingest,
         its value must be "false". All datasets are assumed to be
         ingested except those with this flag. If the dataset should
-        be ingested, no flag should be present.
+        be ingested, no flag (or "true") should be present.
 
         Parameters
         ----------
@@ -560,11 +562,12 @@ class IOOS1_2Check(IOOSNCCheck):
         """
 
         r = True
-        m = "Global attribute \"ioos_ingest\" must be a string with value \"false\""
+        m = ("To disallow harvest of this dataset to IOOS national products, "
+             "global attribute \"ioos_ingest\" must be a string with value \"false\"")
         igst = getattr(ds, "ioos_ingest", None)
-        if igst is not None:
-            if igst != "false":
-                r = False
+        if (isinstance(igst, str) and igst.lower() not in ("true", "false")) or (
+            not isinstance(igst, str) and igst is not None):
+            r = False
 
         return Result(BaseCheck.MEDIUM, r, "ioos_ingest", [m])
 


Test patch:
diff --git a/compliance_checker/tests/test_ioos_profile.py b/compliance_checker/tests/test_ioos_profile.py
index 64dc9e6..fd53ef3 100644
--- a/compliance_checker/tests/test_ioos_profile.py
+++ b/compliance_checker/tests/test_ioos_profile.py
@@ -759,10 +759,18 @@ class TestIOOS1_2(BaseTestCase):
         ds.setncattr("ioos_ingest", "false")
         self.assertTrue(self.ioos.check_ioos_ingest(ds).value)
 
-        # value anything but false
+        # value true
         ds.setncattr("ioos_ingest", "true")
+        self.assertTrue(self.ioos.check_ioos_ingest(ds).value)
+
+        # case insensitive
+        ds.setncattr("ioos_ingest", "True")
+        self.assertTrue(self.ioos.check_ioos_ingest(ds).value)
+        ds.setncattr("ioos_ingest", "False")
+        self.assertTrue(self.ioos.check_ioos_ingest(ds).value)
+
+        # anything else fails
+        ds.setncattr("ioos_ingest", "badval")
         self.assertFalse(self.ioos.check_ioos_ingest(ds).value)
         ds.setncattr("ioos_ingest", 0)
         self.assertFalse(self.ioos.check_ioos_ingest(ds).value)
-        ds.setncattr("ioos_ingest", "False")
-        self.assertFalse(self.ioos.check_ioos_ingest(ds).value)