Eric Blake writes: > On 03/31/2014 01:16 PM, Lluís Vilanova wrote: [...] >> + if not os.path.isabs(include_path): >> + include_path = os.path.join(self.input_dir, >> include_path) >> + if not os.path.isfile(include_path): >> + raise QAPIExprError( >> + expr_info, >> + 'Non-existing included file "%s"' % >> + include_path)
> Is this error necessary, or would you get a sane error message by just > trying to open the file? Not catching it would throw a Python exception, with no context of how the user got there. [...] >> + if include_path in self.included: >> + raise QAPIExprError(expr_info, "Infinite inclusion >> loop: %s" >> + % " -> ".join(self.included + >> + [include_path])) > Good, include-self-cycle.err covers a one-file loop, and > include-cycle.err covers a 3-file loop. > Not so good: your error message is an extremely long line: > +tests/qapi-schema/include-cycle-c.json:1: Infinite inclusion loop: > tests/qapi-schema/include-cycle.json -> > tests/qapi-schema/include-cycle-b.json -> > tests/qapi-schema/include-cycle-c.json -> > tests/qapi-schema/include-cycle.json > The formatting in Benoît's series was a little nicer aesthetically: > +Inclusion loop detected with file: multi_file_loop_include.json > +Path to the broken include is: > + multi_file_loop_include.json > + multi_loop.json > Furthermore, it had the benefit of using the spelling provided by the > user, rather than the absolute path to the files. You want to track > canonical paths for detecting the loop, but do NOT want to report > absolute paths back to the user - instead, it's nicer to report back the > names as they spelled it. I think it's better reporting absolute paths, otherwise the user has to mentally keep track of the relative paths to get to the file. Thanks, Lluis -- "And it's much the same thing with knowledge, for whenever you learn something new, the whole world becomes that much richer." -- The Princess of Pure Reason, as told by Norton Juster in The Phantom Tollbooth
