chore: add checking package file count comparison in release script#547
chore: add checking package file count comparison in release script#547KazuCocoa merged 8 commits intoappium:masterfrom
Conversation
script/release.py
Outdated
| return return_files | ||
|
|
||
| original_files = _get_py_files(APPIUM_DIR_PATH) | ||
| built_files = _get_py_files(BUILT_APPIUM_DIR_PATH) |
There was a problem hiding this comment.
we probably don't have to return file names, paths, but only counts
There was a problem hiding this comment.
I considered which was better, but wanted to check the list when some packages was missed. The listed error message helped me to see the diff in #542 case. No strong opinion, but I won't write the similar code to get files when error happens for now. (Current count is not so huge like over 100)
There was a problem hiding this comment.
makes sense. In such case I'd consider returning sets instead and only show the actual difference between these sets (using difference helper)
script/release.py
Outdated
|
|
||
| def _get_py_files(root_dir: str) -> List[str]: | ||
| def get_py_files_in_dir(root_dir: str) -> List[str]: | ||
| _files = [] |
There was a problem hiding this comment.
please remove all these starting underscores
script/release.py
Outdated
|
|
||
| return True | ||
| diff = set(original_files).difference(set(built_files)) | ||
| if diff != set(): |
There was a problem hiding this comment.
simply if diff. Empty set is considered falsy
script/release.py
Outdated
| if diff != set(): | ||
| print(f"'{APPIUM_DIR_PATH}' has '{diff}' files than {BUILT_APPIUM_DIR_PATH}") | ||
| diff = set(built_files).difference(set(original_files)) | ||
| if diff != set(): |
script/release.py
Outdated
| diff = set(original_files).difference(set(built_files)) | ||
| if diff != set(): | ||
| print(f"'{APPIUM_DIR_PATH}' has '{diff}' files than {BUILT_APPIUM_DIR_PATH}") | ||
| diff = set(built_files).difference(set(original_files)) |
There was a problem hiding this comment.
both sets could be precalculated
script/release.py
Outdated
| if not has_same_file_count_original_built(): | ||
| exit("Python files in 'build/lib/appium' directory may differ from 'appium'. " | ||
| "Please make sure setup.py is configured properly.") | ||
| compare_file_count_in_package() |
There was a problem hiding this comment.
I'd say assert_files_count_in_package
ki4070ma
left a comment
There was a problem hiding this comment.
You could simplify, but it's ok.
| exit(f'Failed to build the package:\n{output}') | ||
|
|
||
|
|
||
| def get_py_files_in_dir(root_dir: str) -> List[str]: |
There was a problem hiding this comment.
You can do the same thing as below. (maybe same?)
import glob
import os
files = glob.glob(f'{root_dir}/**/*.py', recursive=True) + glob.glob(f'{root_dir}/**/*.typed', recursive=True)
return [os.path.basename(x) for x in files]There was a problem hiding this comment.
ah, recursive is available in 3
script/release.py
Outdated
| original_files = get_py_files_in_dir(APPIUM_DIR_PATH) | ||
| built_files = get_py_files_in_dir(BUILT_APPIUM_DIR_PATH) | ||
|
|
||
| print(len(original_files)) |
There was a problem hiding this comment.
Please also mention what is printed: f'Original files count: {len(original_files)}'
There was a problem hiding this comment.
oops, this is my debugging...
| original_files_set = set(original_files) | ||
| built_files_set = set(built_files) | ||
|
|
||
| diff = original_files_set.difference(built_files_set) |
There was a problem hiding this comment.
There was a problem hiding this comment.
ah, often used the way in Ruby. Cool
| base_dir_path_count = len(root_dir) | ||
| return [_file[base_dir_path_count:] for _file in _files] | ||
| return [ | ||
| file_path[len(root_dir):] |
It help to prevent broken package like #542 without any expensive process.