John Snow <js...@redhat.com> writes: > On 9/25/20 7:34 AM, Markus Armbruster wrote: >> Cleber Rosa <cr...@redhat.com> writes: >> >>> On Tue, Sep 22, 2020 at 05:00:25PM -0400, John Snow wrote: >>>> This is a minor re-work of the entrypoint script. It isolates a >>>> generate() method from the actual command-line mechanism. >>>> >>>> Signed-off-by: John Snow <js...@redhat.com> [...] >>>> +def generate(schema_file: str, >>>> + output_dir: str, >>>> + prefix: str, >>>> + unmask: bool = False, >>>> + builtins: bool = False) -> None: >>>> + """ >>>> + generate uses a given schema to produce C code in the target >>>> directory. >>>> + >>>> + :param schema_file: The primary QAPI schema file. >>>> + :param output_dir: The output directory to store generated code. >>>> + :param prefix: Optional C-code prefix for symbol names. >>>> + :param unmask: Expose non-ABI names through introspection? >>>> + :param builtins: Generate code for built-in types? >>>> + >>>> + :raise QAPIError: On failures. >>>> + """ >>>> + match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix) >>>> + if match and match.end() != len(prefix): >>> >>> Nice catch with the extra check here. Maybe worth mentioning and/or >>> splitting the change? >> >> Please do not sneak additional checking into patches advertized as pure >> refactoring. It makes me look for more sneakery with a microscope. >> >> This re.match() cannot possibly fail. Three cases: >> >> * First character is funny >> >> The regexp matches the empty string. There's a reason the regexp ends >> with '?'. >> >> * Non-first character is funny >> >> The regexp matches the non-funny prefix. >> >> * No character is funny >> >> The regexp matches the complete string. >> >> Checking impossible conditions as if they were possible is confusing. >> Please drop the additional check. >> >> We can talk about checking this impossible condition with >> >> assert(match) >> >> if you believe it makes the code easier to understand (it does not >> improve its behavior). > > My use of strict_optional=False is what prevents this from exhibiting > as an error in mypy. An assert will help convince mypy that 'match' > cannot possibly be 'None'.
Adding assertions to help mypy along is okay. > eh, well. I will fix this when I remove strict_optional, so I will > just remove this additional check for now to avoid adding another > patch to this series. Makes sense to me.
