Refactor yacfg Codebase and Update Project Dependencies #229
Conversation
| """ | ||
| Get module installation path, |
There was a problem hiding this comment.
I thought that """Get (quotes and first word on a single line) is the generally preferred style. But OK, whatever. Let's turn on black in GHA some time next week anyways, or next month ;P
There was a problem hiding this comment.
Indeed, I do find D213 and D212 in pydocstyle a bit challenging at the moment. By the way, I already utilize black for every commit. 😸
| """Get module installation path, | ||
| def get_module_path() -> str: | ||
| """ | ||
| Get module installation path, | ||
| for reading packaged template sets and profiles | ||
|
|
||
| :return: module installation path | ||
| :rtype: str |
There was a problem hiding this comment.
Duplicating the type annotation in a type comment is poor practice. I hope subsequent commits fix this!
There was a problem hiding this comment.
Yes, you're right. I should go through everything. I'm really excited about it. 😁
There was a problem hiding this comment.
i wouldn't block this PR on it, or you won't ever be done... do find/replace later, you can just delete lines matching regex, I think
| "unix": time.time(), | ||
| }, | ||
| } | ||
|
|
||
|
|
||
| def add_render_config(config_data, render_options): | ||
| """add render related config to original template data | ||
| def add_render_config(config_data: Dict, render_options: "RenderOptions") -> None: |
There was a problem hiding this comment.
from __future__ import annotations should be available in 3.7+, so that could be used, to avoid the quotes. (https://docs.python.org/3/library/__future__.html)
There was a problem hiding this comment.
Hey, you're absolutely right! I actually overlooked quite a few things. Thanks for being an extra set of eyes! I appreciate it!
| or None to do a dry run | ||
| :type output_path: str | ||
| :type output_path: str | None |
There was a problem hiding this comment.
There's some compatibility package to allow using | instead of Union[] in older Pythons. I think that it should be adopted in the future, so we can have modern type annotations.
There was a problem hiding this comment.
I looked. So, the PEP is https://peps.python.org/pep-0604/, and it comes with Python 1.10. When you do from future import annotations, you can use the syntax in previous pythons, because then it's not being evaluated at runtime python/mypy#9610 (comment) And mypy supports this, apparently (same gh issue).
So, no package necessary, unless you want to do those type aliases, I guess. I remember seeing such package, but don't recall what it was called.
There was a problem hiding this comment.
Yeah, I'm facing the same situation. I should search for an alternative tool that can analyze all the docstrings. Moreover, since we have typing, we can consider removing this information from the docstrings altogether.
There was a problem hiding this comment.
Nobody reads the generated docs anyways, I think it's ok not to have types there for now. It's IDE where it matters.
Sphynx needed this, iirc, https://pypi.org/project/sphinx-autodoc-typehints/. That jupyter book thing I brought in should work with it somehow, https://jupyterbook.org/en/stable/explain/sphinx.html
If you want to leave that up to me, I can try looking, since I'm who introduced jupyter book in first place
Thanks for checking out the stuff, If there's something you didn't understand or if you have any questions, just let me know. I'm happy to explain things better or give you more info. I've been refactoring and refactoring... It would deserve even more sincere attention, but we don't have the time to completely rewrite it from scratch. |
Somebody (@gtully, et al) will rewrite this in Java anyways, to configure broker from properties, without init image that carres python... Ideally, being able to configure fully a standalone instance (or cluster) conveniently, with single config file, and have the tool understand the schema defined in Java code would be nice... not sure if there's something to build it upon, though. I do not want to have to have the top level template in XML ;( Maybe there would be some desire for ansible for this? I know that skupper resolved the Python 3.9 vulnerability on RHEL 8 by upgrading the image to RHEL 9, where Python 3.9 is patched (which IMO would've been sensible for broker too, but 3.8 there, that's already done deal). And skupper will have or has some ansible in upstream repo. So, that actually means it's back to jinja and python, because ansible ;P |
|
When can we merge this? |
jiridanek
force-pushed
the
refactor_2023
branch
from
15c24b9
to
0e4ee78
Compare
Refactor the files.py module to implement type annotations using mypy. - Updating the module path retrieval method in the get_module_path() function. - Refactoring the profiles and templates paths retrieval methods in the get_profiles_path() and get_templates_path() functions. - Modifying the select_profile_file() and select_template_dir() functions to handle user-defined paths and improve logging. - Adding type annotations to function parameters and return types. - Refactoring the ensure_output_path() function to handle existing output paths correctly. These changes aim to improve the functionality and maintainability of the files.py module.
- Refactoring the config_console_logger() function to improve code structure and readability. - Adding type annotations to function parameters and return types. - Setting the default logger level to DEBUG. - Implementing a file logging feature using a rotating file handler, allowing log files to rotate based on size and keeping a specified number of backup log files. These changes aim to improve the functionality and maintainability of the logger_settings.py module.
Refactor the config_data.py module. - Adding type annotations to function parameters and return types. - Refactoring the add_template_metadata() function to improve code structure and readability. - Refactoring the add_render_config() function to improve code structure and readability. - Implementing the RenderOptions named tuple to hold render-related options. - Updating the config_data module to include template metadata and render configuration in the original template data. These changes aim to enhance the functionality and maintainability of the config_data.py module.
Refactor the exceptions.py module. - Refactoring the YacfgException class to serve as the base exception for all YACFG-related exceptions. - Refactoring the TemplateError, ProfileError, GenerationError, ConfigurationError, ParsingError, AuthorizationError, and InvalidInputError classes to inherit from the YacfgException base class. - Adding docstrings to each exception class to provide information about the specific type of error it represents. - Implementing the InvalidInputError class with additional attributes for the input value that caused the error and an error message. These changes aim to improve maintainability of the exceptions.py module.
This commit refactors the query.py module. - Adding type annotations to function parameters and return types. - Refactoring the filter_template_list() function to use list comprehension and improve code readability. - Refactoring the list_templates() function to generate the template paths using os.walk() and relpath() instead of relying only on the presence of a file named "_template". - Refactoring the list_profiles() function to generate the profile paths using os.walk() and relpath(), while excluding directories starting with an underscore and filtering only YAML files. - Refactoring the get_main_template_list() function to use a regex pattern to filter out main templates from the list of templates provided by the Jinja2 environment. These changes aim to improve the maintainability, and performance of the query.py module.
Refactors the output.py module. - Adding type annotations to function parameters and return types. - Refactoring the MyDumper class to inherit from yaml.SafeDumper and adjust the increase_indent() method. - Refactoring the yaml_dump_wrapper() function to improve code structure and readability. - Refactoring the new_profile() function to use the files module for file operations and handle exceptions appropriately. - Refactoring the new_profile_rendered() function to use the profiles module for profile handling and data extraction. - Refactoring the new_template() function to use the files module for file operations and handle exceptions appropriately. - Refactoring the export_tuning_variables() function to use the profiles module for profile handling and data extraction. - Refactoring the write_output() function to improve code structure and readability. These changes aim to enhance the functionality and maintainability of the output.py module.
Refactor the metadata and descriptions for the 'yacfg' and 'yacfg-batch' packages.
Refactor the file path and directory handling in the `files.py` module. The following changes have been made: - Import statements have been reorganized. - The `REX_TEMPLATE_TO_OUTPUT` regular expression has been removed. - The `get_profiles_path()` function has been renamed to `get_paths()` and generalized to handle different types of paths. - The `get_templates_path()` function has been renamed to `get_paths()` and generalized to handle different types of paths. - The `select_profile_file()` function has been updated to utilize the `get_profiles_paths()` function and simplified. - The `select_template_dir()` function has been updated to utilize the `get_templates_paths()` function and simplified. - The `ensure_output_path()` function has been updated to handle `NotADirectoryError` instead of `OSError`. - The `get_output_filename()` function has been simplified using regular expressions. These refactorings improve the code's readability, maintainability, and modularity.
Refactor the file handling in the `output.py` module. - Import statements have been reorganized. - The `write_output()` function has been updated to use a more descriptive parameter name (`content` instead of `data`). - The `write_output()` function has been updated to handle `OSError` when there is a problem with the output path or writing the file.
Refactor the profile handling in the `profiles.py` module. - Import statements have been reorganized. - The license header has been removed. - The `load_tuning_files()` function has been updated to use `yaml.safe_load()` instead of `yaml.load()` to load the tuning files. - `get_profile_template()`, `load_profile_defaults()`, `get_tuned_profile()`, load_tuning() functions has been updated to utilize type annotations and use default values for the parameters. These refactorings improve the code's readability, maintainability, and type safety, ensuring that profile handling operations are clear and any potential errors are handled appropriately.
- Updated Python minimum required version to 3.8 in the `README.md` file - Corrected the name of the maintainer in the `README.md` file - Updated the license year to 2023 in the `README.md` file - Optimized imports and removed unused imports in `config_data.py` - Improved logger settings and added debug log messages in `logger_settings.py` - Enhanced query functionality and added debug log messages in `query.py` - Updated the paths for profiles and templates in the tests This commit aims to improve the codebase, enhance performance, and ensure accurate information in the package documentation and tests.
- Removed unused imports and code. - Improved error handling and logging in the `generate_outputs` function. - Added type hints to function parameters and return types. Refactor the `yacfg.py` module by removing unnecessary code, improving error handling, and enhancing the overall code structure for better readability and maintainability.
- Rearranged import statements for better organization - Removed unused import statements - Updated variable names for clarity and consistency - Fixed indentation and formatting issues - Added type annotations to function parameters and return types - Replaced multiple instances of logging error messages with a single error function - Handled exceptions in the main function for better error handling - Reorganized and simplified conditional statements for different actions - Adjusted logging levels based on command line options - Improved error messages for better clarity - Refactored the generation process for improved performance and maintainability
…_batch.py, for improved code organization and readability - Update and remove unused import statements - Created separate functions for boolize, split_key_value, and parse_key_value_list - Added type annotations to function parameters and return types - Reorganized code to improve clarity and maintainability
- Updated the dependencies in the poetry.lock file to their latest compatible versions - Ensured compatibility and stability of the project dependencies
- Updated function signature to include type hint for template_name parameter and specify return type as Environment. - Revised docstring to provide detailed information about function purpose, parameters, return type, and potential exceptions. - Refactored code to use ChoiceLoader and FileSystemLoader for template loading.
…ate_dir function - Simplified the logic for selecting the template directory. - Directly checked if the user-defined template path without the "templates" directory exists. - Logged and returned the user-specified template directory if it exists. - Iterated through templates_paths to find a matching template directory with a "_template" file. - Logged and returned the selected template directory if found. - Raised a TemplateError if no matching template directory was found.
Updated regular expressions for boolean conversion to use case-insensitive matching
- Import the `metadata` module from `importlib_metadata` if the Python version is less than 3.10, and from `importlib` otherwise. - Adjust file structure by renaming `src/yacfg/cli_arguments.py` to `src/yacfg/cli/cli_arguments.py` and `src/yacfg/yacfg_cli.py` to `src/yacfg/cli/yacfg_cli.py`. - Improve and fix issues in the `yacfg_cli` module related to logging settings, command-line option handling, and calling appropriate functions based on the provided arguments.
- Python "^3.8.1" - flake8 "^6.0.0" - tox "^4.6.3" - pre-commit version requirement to "^3.3.0" Update the script entry for yacfg to 'yacfg.cli.yacfg_cli:main'
The 'parse_key_value_list' function in cli_arguments.py file was refactored to accommodate a different input and more accurate return type. Previously, it was looking for a list of dictionaries as an input but that has been revised to accept a list of strings with the format "KEY=VALUE". This function now returns a dict of split key-value options. The imported __future__ annotations has also been added in this version.
This commit changes the paths of mocked objects in tests to reflect updates in the 'files' and 'profiles' modules of 'yacfg'. Fake file and profile paths have been updated accordingly. Additionally, improvements have been made to the logic of handling underscore-prefixed paths in 'query.py'. The 'select_profile_file' function in 'files.py' has also been updated for more accurate profile path selection.
The previous tests on rendering config data and ensuring output paths were overly complex and tested certain scenarios that were not necessary. This commit simplifies these tests by only testing for the presence and correct value of keys in the render config and removes redundant mock path tests. Additionally, changes have been made to improve the handling of paths with underscores. This simplification and improvement of test coverage increases the maintainability and reliability of the code.
Previously, the profile name was incorrectly assigned where the full path was used instead of just the base name. This led to issues when trying to locate the file on the system. This update changes the assignment to use the base name retrieved from the full path, ensuring the correct file is always referenced.
The original condition checked whether both profile and template options are selected which created confusion and redundancy. We have simplified it by only checking the profile option. We have also removed the "__main__" function call since it wasn't used anywhere in the file and doesn't affect the correctness, but it does reduce the unnecessary clutter. By making these changes, we're anticipating a clearer and cleaner code.
Incorporated the sys module to handle any exceptions in 'GenerationError'. Previously, the code would return an empty dictionary and continue executing. To improve error handling, day-to-day operations and halt execution when error occurs, the sys.exit(1) method is added. This ensures the program execution stops immediately and is especially useful in production environments where continuous execution after error could lead to undesirable outcomes. NOTE: Commit message is off-topic to the provided diff.
Added a debug log to display the name of the selected template in the 'profiles.py' file. Previously, it only logged the selected profile path. The logging of the template name will enhance debugging, making it quicker and easier for developers to identify the template causing any issues.
jiridanek
force-pushed
the
refactor_2023
branch
from
0e4ee78
to
640b5eb
Compare
Description
This pull request encompasses a wide range of refactoring updates for various yacfg modules aimed at improving code readability, maintainability, performance, and type safety. Updates include the addition of type annotations to function parameters and return types, simplification of code logic, reorganization of import statements, and improved exception handling.
Here is an overview of the changes made:
pyproject.tomlhave been updated, and the script entry for yacfg has been modified.yacfg_cli,yacfg_batch_cli,cli_arguments,yacfg_batch,output,query,profiles,output,exceptionshave been significantly refactored for improved readability, maintainability, and performance.files.py,output.py,profiles.pymodules respectively.yacfgandyacfg-batchpackages.README.mdfile and documentation have been updated with accurate information.poetry.lockfile updated to ensure compatibility and stability of the project dependencies.Each commit message has further details of the changes made.