[GitHub] [beam] TheNeuralBit commented on a change in pull request #12857: [BEAM-9547] Add not_implemented_ok

[GitBox <gi...@apache.org>]

TheNeuralBit commented on a change in pull request #12857:
URL: https://github.com/apache/beam/pull/12857#discussion_r490376616



##########
File path: sdks/python/apache_beam/dataframe/doctests.py
##########
@@ -316,31 +354,39 @@ def run(self, test, **kwargs):
         example.source = 'pass'
         example.want = ''
         self.skipped += 1
-      elif example.exc_msg is None and any(
-          wont_implement(example)
-          for wont_implement in self._wont_implement_ok.get(test.name, [])):
+      elif example.exc_msg is None and self._is_wont_implement_ok(example,
+                                                                  test):
+        # Don't fail doctests that raise this error.
+        example.exc_msg = '%s: ...' % WONT_IMPLEMENT
+      elif example.exc_msg is None and self._is_not_implemented_ok(example,

Review comment:
       > does the contents of this really matter anymore?
   
   Do you mean the contents of `exc_msg`? It sort of matters because it's passed back to us in "want" in the output checker, then we can distinguish which exception is ok. But it is worth noting that for both of these cases the exc_msg is not actually being used by doctests as one might expect, since we short circuit it in the output checker. I think we could put whatever we want in the exc_msg.
   
   > What about doctests where both wont_implement_error and not_implemented_ok are True?
   
   Yeah overlap between the three sets is not handled well, there's just a precedence `skip > wont_implement_ok > not_implemented_ok`. Maybe there should be logic to enforce there's no overlap between wont_implement/not_implemented, I don't think we should allow one example to throw either error.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org