gtristan commented on code in PR #2089:
URL: https://github.com/apache/buildstream/pull/2089#discussion_r2510234111


##########
src/buildstream/_yaml.pyx:
##########
@@ -293,10 +293,10 @@ cpdef MappingNode load(str filename, str shortname, bint 
copy_tree=False, object
 cpdef MappingNode load_data(str data, int 
file_index=node._SYNTHETIC_FILE_INDEX, str file_name=None, bint 
copy_tree=False):
     cdef Representer rep
 
-    try:

Review Comment:
   Ideally, we would have the precise exception below instead of the blind 
`except Exception` statement.
   
   Given that determining what exception specifically is raised there may be 
quite difficult, I think I'd still be more comfortable with an additional 
`except ImportError` block, so that anything else happening inside the 
`yaml.CParser(data)` constructor is still in the `try` block.
   
   E.g. just adding the following before the blind except may be preferrable:
   
   ```python
   try:
      ...
   except ImportError as e:
       raise e
   except Exception:
      ... keeping the same blind except...
   ``



-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to