potiuk commented on code in PR #30775: URL: https://github.com/apache/airflow/pull/30775#discussion_r1173976889
########## scripts/ci/pre_commit/pre_commit_unittest_testcase.py: ########## @@ -0,0 +1,59 @@ +#!/usr/bin/env python +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import re +import sys +from pathlib import Path + +from rich.console import Console + +if __name__ not in ("__main__", "__mp_main__"): + raise SystemExit( + "This file is intended to be executed as an executable program. You cannot use it as a module." + f"To run this script, run the ./{__file__} command [FILE] ..." + ) + + +console = Console(color_system="standard", width=200) + + +def _check_file(file: Path) -> list: + content = file.read_text() + return re.findall(r"class[^(]+\(unittest.TestCase\)\:", content) Review Comment: NIT: I am fine with using regexp, but as the old Chinese proverb says "you have problem - introduce, regexp, now you have two problems". Why don't we use AST for that ? We have a couple of other pre-commits that use AST parsing of Python files, and while esoteric a bit for people, it's not as difficult as it might seem. We have some examples there that migh help with getting the right parser. Ths has the nice benefit that it could also get the error message including source and line code (which are present in the AST tree I believe) and that it will always work even if you have long class name which will lead your class definition to be broken across mulitple lines. ########## scripts/ci/pre_commit/pre_commit_unittest_testcase.py: ########## @@ -0,0 +1,59 @@ +#!/usr/bin/env python +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import re +import sys +from pathlib import Path + +from rich.console import Console + +if __name__ not in ("__main__", "__mp_main__"): + raise SystemExit( + "This file is intended to be executed as an executable program. You cannot use it as a module." + f"To run this script, run the ./{__file__} command [FILE] ..." + ) + + +console = Console(color_system="standard", width=200) + + +def _check_file(file: Path) -> list: + content = file.read_text() + return re.findall(r"class[^(]+\(unittest.TestCase\)\:", content) Review Comment: NIT: I am fine with using regexp, but as the old Chinese proverb says "you have problem - introduce regexp, now you have two problems". Why don't we use AST for that ? We have a couple of other pre-commits that use AST parsing of Python files, and while esoteric a bit for people, it's not as difficult as it might seem. We have some examples there that migh help with getting the right parser. Ths has the nice benefit that it could also get the error message including source and line code (which are present in the AST tree I believe) and that it will always work even if you have long class name which will lead your class definition to be broken across mulitple lines. -- 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]
