parse profile env lines #90
Conversation
WalkthroughThis update brings various enhancements across different files in the project, including adding new configuration keys, refining logic in methods, introducing default arguments, and improving error handling. These changes aim to streamline processes, enhance flexibility, and ensure smoother functionality within the project. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| if (n[STR_ENV].IsScalar()) { | ||
| for (auto const& line : mkn::kul::String::LINES(n[STR_ENV].Scalar())) | ||
| evs.emplace_back(PARSE_ENV_NODE(decltype(n[STR_ENV]){line}, *this)); | ||
| } else |
There was a problem hiding this comment.
The modification to the populateMaps function introduces a loop to iterate over lines if the scalar value contains multiple lines. This is a significant improvement for parsing environment variables or configurations that span multiple lines. However, there are a few considerations:
-
Correctness and Logic: The approach correctly checks if
n[STR_ENV]is a scalar and then iterates over each line usingmkn::kul::String::LINES. This ensures that multi-line scalar values are properly handled. The use ofPARSE_ENV_NODEfor each line is appropriate for parsing individual environment variables. -
Performance: Iterating over each line and parsing them individually might introduce a slight overhead compared to the previous single-pass parsing. However, given the context (parsing configuration files), this overhead is likely negligible and acceptable for the added flexibility and correctness.
-
Maintainability: The code is relatively straightforward and enhances the maintainability by clearly handling a previously unaddressed case (multi-line scalar values). The separation of logic for scalar and non-scalar cases is clean and easy to follow.
-
Error Handling: It's assumed that
PARSE_ENV_NODEandmkn::kul::String::LINEShave their own error handling mechanisms. However, it would be beneficial to ensure that any potential exceptions or errors in parsing individual lines are appropriately handled or logged, especially since environment variable parsing is critical for application configuration. -
Testing: It's crucial to ensure that this new functionality is covered by unit tests, especially for edge cases such as empty lines, lines with only whitespace, or improperly formatted environment variables.
Overall, the changes are a positive enhancement to the populateMaps function, addressing a specific limitation in the previous implementation. Ensuring thorough testing and considering the above points will help maintain the robustness of this functionality.
Would you like assistance in adding unit tests for this new functionality, especially focusing on edge cases?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 13
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
mkn.yamlis excluded by:!**/*.yaml
Files selected for processing (15)
- README.noformat (6 hunks)
- inc/maiken/app.hpp (1 hunks)
- inc/maiken/defs.hpp (3 hunks)
- inc/maiken/project.hpp (2 hunks)
- inc/maiken/scm.hpp (1 hunks)
- inc/maiken/settings.hpp (1 hunks)
- src/maiken/build/bin.cpp (3 hunks)
- src/maiken/compiler/gcc.cpp (2 hunks)
- src/maiken/defs.cpp (1 hunks)
- src/maiken/depmod.cpp (1 hunks)
- src/maiken/mods.cpp (2 hunks)
- src/maiken/process.cpp (2 hunks)
- src/maiken/setup.cpp (3 hunks)
- src/project.cpp (3 hunks)
- src/settings.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- src/maiken/defs.cpp
Additional comments: 11
src/maiken/build/bin.cpp (2)
- 139-142: The changes in the
buildExecutablemethod adjust the order of operations, specifically moving the error checking (checkErrors) and command output (KOUT(INF)) statements. While reordering these operations can improve readability or logical flow, it's crucial to ensure that it does not affect the functionality or error handling capabilities of the method. The current changes appear to maintain the method's integrity, but it's important to test these changes thoroughly to confirm that error handling and output logging behave as expected in all scenarios.- 210-210: In the
buildLibrarymethod, theExecutioner::printcall has been moved to occur before thecheckErrorscall. This change aligns with the adjustments made in thebuildExecutablemethod, aiming to standardize the order of operations across methods. As with thebuildExecutablemethod, it's important to verify that this reordering does not negatively impact the method's error handling and output logging. Comprehensive testing is recommended to ensure the method functions correctly after these changes.src/maiken/depmod.cpp (1)
- 105-105: Replacing the hardcoded string "master" with the
defaultSCMBranchName()function call is a significant improvement in terms of flexibility and adaptability. This change allows the application to dynamically determine the default branch name based on the SCM system in use, accommodating different default branch naming conventions (e.g., "main" in recent GitHub repositories). It's important to ensure that thedefaultSCMBranchName()function is implemented to accurately reflect the default branch names of supported SCM systems and that it's thoroughly tested to handle edge cases and exceptions.src/maiken/compiler/gcc.cpp (2)
- 34-34: Including
<cstdint>ensures that thestd::uint16_ttype is correctly recognized by the compiler. This change aligns with best practices for using fixed-width integer types in C++.- 203-203: Changing
uint16_ttostd::uint16_tis a good practice for ensuring portability and clarity in type usage. This change correctly uses the C++ standard library's fixed-width integer types.src/settings.cpp (1)
- 145-145: Adjusting the namespace for the return type of the
validatormethod tomkn::kul::yaml::Validatorimproves clarity and ensures consistency in namespace usage. This change is appropriate and aligns with best practices for namespace management in C++.src/maiken/setup.cpp (1)
- 94-94: The call to
sub_initializerwithinApplication::setupcorrectly integrates the new initialization logic into the application setup process. This ensures that the necessary initialization tasks are performed at the appropriate stage of the setup. Ensure that any dependencies or prerequisites forsub_initializerare met before this call.inc/maiken/app.hpp (1)
- 312-312: The addition of the
subsmember variable to theApplicationclass is a logical extension to support handling subprojects or dependencies more effectively. Ensure that:
- Initialization: The
subsvector is properly initialized where necessary, especially in constructors or methods that reset or reconfigure theApplicationinstance.- Usage: The
subsvector is utilized appropriately throughout the codebase, with operations on it being performed in a thread-safe manner if concurrent access is possible.- Memory Management: Given that
subsstoresProjectInfoobjects, ensure that any dynamic memory associated with these objects is managed correctly to avoid memory leaks.README.noformat (3)
- 33-35: The addition of a distributions section with a link to binaries on GitHub is helpful for users looking for pre-compiled versions of the software. This change improves accessibility to the software.
- 174-174: Updating advice on using the mode "none" for applications with both static and shared dependencies is crucial for avoiding potential build issues. This clarification helps users make informed decisions about their project configurations.
- 410-410: Correcting the typo in the
MKN_DBGenvironment variable setting for gdb improves clarity and prevents confusion. Typos in documentation, especially related to configuration or environment variables, can lead to significant user frustration.
| NodeValidator("out"), | ||
| NodeValidator("ext"), | ||
| NodeValidator("self"), | ||
| NodeValidator("sub"), | ||
| NodeValidator("with"), | ||
| env, | ||
| dep, |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [62-148]
The modifications to the validator function include the addition of new validation nodes such as "sub". This expansion of validation criteria is a positive change, ensuring more comprehensive validation of project configurations. A few points to consider:
- The addition of new nodes increases the complexity of the validation process. It's important to ensure that the documentation is updated accordingly to reflect these changes and guide users on how to comply with the new validation rules.
- The use of hardcoded strings for node names (e.g., "name", "version", "profile") could be replaced with constants to reduce the risk of typos and make the code easier to maintain.
- The structure of the validation rules is clear and follows a consistent pattern, which is good for maintainability. However, consider if there's a way to simplify or modularize this further, especially if more nodes are expected to be added in the future.
Overall, the changes to the validator function are beneficial for enhancing the project's validation capabilities. Ensuring good documentation and considering the use of constants for node names could further improve the code quality.
| Description Global override, if set forces all compile calls to use value. Example, low RAM systems | ||
|
|
||
| Key MKN_DEFAULT_BRANCH | ||
| Type String | ||
| Default "master" | ||
| Description Default branch for SCM if not given or set | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
The search results confirm that MKN_COMPILE_THREADS and MKN_DBG are indeed handled in the application code, as indicated by their presence in src/maiken/run.cpp and src/maiken/create.cpp. However, there is no evidence of MKN_DEFAULT_BRANCH being handled or used in the application code based on the provided search results. This could imply that the addition of MKN_DEFAULT_BRANCH to the documentation might not be fully implemented in the code, or it is used in a context not covered by the search parameters.
Given the information at hand, it's advisable to further investigate the implementation or usage of MKN_DEFAULT_BRANCH within the application to ensure full configurability as documented.
Analysis chain
The addition of new configuration keys MKN_DEFAULT_BRANCH and updates to descriptions for existing keys like MKN_COMPILE_THREADS and MKN_DBG enhance the configurability and usability of the tool. It's important to ensure that these keys are also reflected in the application's code where they are used or parsed.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the new configuration keys are handled in the application code
rg 'MKN_DEFAULT_BRANCH|MKN_COMPILE_THREADS|MKN_DBG' src/
Length of output: 386
| @@ -53,13 +59,13 @@ | |||
| Project(mkn::kul::File const& f) : mkn::kul::yaml::File(f), m_dir(f.dir().real()) {} | |||
| Project(Project const& p) : mkn::kul::yaml::File(p), m_dir(p.m_dir.real()) {} | |||
| mkn::kul::Dir const& dir() const { return m_dir; } | |||
| const mkn::kul::yaml::Validator validator() const; | |||
| mkn::kul::yaml::Validator validator() const; | |||
There was a problem hiding this comment.
Similar to the change in settings.hpp, removing the const qualifier from the return type of the validator method in the Project class allows modifications to the returned mkn::kul::yaml::Validator object. Evaluate this change to ensure it's necessary for the intended use cases and does not introduce risks of unintended modifications.
| mkn::kul::String::TRIM(mod); | ||
| if (mod.empty()) continue; | ||
|
|
||
| mod_nodes.emplace_back(); | ||
| auto& node = mod_nodes.back(); | ||
| std::string local /*&*/, profiles, proj = mod, version /*#*/, scm, objs; | ||
| auto& node = mod_nodes.emplace_back(); | ||
|
|
||
| auto get_between = [&](auto& var, auto lbrak, auto rbrak) { | ||
| auto between = maiken::string::between_rm_str(proj, lbrak, rbrak); | ||
| if (between.found) proj = between.remaining, var = *between.found; | ||
| return !between.error; | ||
| }; | ||
| ProjectInfo pInfo = ProjectInfo::PARSE_LINE(mod); | ||
|
|
||
| if (!get_between(scm, "(", ")")) KEXIT(1, "Invalid -m - missing right ) bracket"); | ||
| if (!node[STR_SCM]) node[STR_SCM] = scm; | ||
| auto& [local, profiles, proj, version, scm] = pInfo; | ||
| // KLOG(INF) << proj; | ||
| std::string objs; | ||
|
|
||
| if (!get_between(profiles, "[", "]")) KEXIT(1, "Invalid -m - missing right ] bracket"); | ||
| mkn::kul::String::REPLACE_ALL(profiles, ",", " "); | ||
| if (!node[STR_SCM] && scm.size()) node[STR_SCM] = scm; | ||
| if (!node[STR_PROFILE] && profiles.size()) node[STR_PROFILE] = profiles; | ||
|
|
||
| { |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [64-95]
The refactoring of the mod method to use ProjectInfo::PARSE_LINE for parsing project information simplifies the logic for handling various project fields (local, profiles, proj, version, and scm). This approach enhances code clarity and maintainability by centralizing the parsing logic. Ensure that the ProjectInfo::PARSE_LINE method is thoroughly tested to handle all expected input formats correctly.
| mkn::kul::hash::map::S2S oldEvs; | ||
| for (auto const& ev : evs) { | ||
| std::string const& v = mkn::kul::env::GET(ev.name()); | ||
| if (v.size()) oldEvs.insert(ev.name(), v); | ||
| mkn::kul::env::SET(ev.name(), ev.value()); | ||
| } |
There was a problem hiding this comment.
The addition of code to store and restore environment variables before and after executing commands in the process() function is a good practice for maintaining a stable environment. Ensure that this mechanism robustly handles exceptions or errors during command execution to guarantee that environment variables are always correctly restored.
Also applies to: 145-145
| @@ -78,6 +79,9 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | |||
| #endif /* _KUL_PROC_LOOP_NSLEEP_ */ | |||
|
|
|||
| namespace maiken { | |||
|
|
|||
| auto inline defaultSCMBranchName() { return mkn::kul::env::GET("MKN_DEFAULT_BRANCH", "master"); } | |||
There was a problem hiding this comment.
The addition of the defaultSCMBranchName inline function provides a flexible way to determine the default SCM branch name, using the MKN_DEFAULT_BRANCH environment variable or defaulting to "master". This approach enhances configurability and should be well-documented to ensure users are aware of how to use this feature.
| @@ -124,6 +128,7 @@ | |||
| static constexpr auto STR_REMOTE = "remote"; | |||
| static constexpr auto STR_SCM = "scm"; | |||
| static constexpr auto STR_SELF = "self"; | |||
| static constexpr auto STR_SUB = "sub"; | |||
There was a problem hiding this comment.
The introduction of the STR_SUB constant in the Constants class suggests the addition of a new feature or configuration option related to "sub" entities. Ensure that this constant is consistently used across the codebase and that its purpose is clearly documented.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
mkn.yamlis excluded by:!**/*.yaml
Files selected for processing (15)
- README.noformat (6 hunks)
- inc/maiken/app.hpp (1 hunks)
- inc/maiken/defs.hpp (3 hunks)
- inc/maiken/project.hpp (2 hunks)
- inc/maiken/scm.hpp (1 hunks)
- inc/maiken/settings.hpp (1 hunks)
- src/maiken/build/bin.cpp (3 hunks)
- src/maiken/compiler/gcc.cpp (2 hunks)
- src/maiken/defs.cpp (1 hunks)
- src/maiken/depmod.cpp (1 hunks)
- src/maiken/mods.cpp (2 hunks)
- src/maiken/process.cpp (2 hunks)
- src/maiken/setup.cpp (3 hunks)
- src/project.cpp (3 hunks)
- src/settings.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (15)
- README.noformat
- inc/maiken/app.hpp
- inc/maiken/defs.hpp
- inc/maiken/project.hpp
- inc/maiken/scm.hpp
- inc/maiken/settings.hpp
- src/maiken/build/bin.cpp
- src/maiken/compiler/gcc.cpp
- src/maiken/defs.cpp
- src/maiken/depmod.cpp
- src/maiken/mods.cpp
- src/maiken/process.cpp
- src/maiken/setup.cpp
- src/project.cpp
- src/settings.cpp
PhilipDeegan
force-pushed
the
env
branch
2 times, most recently
from
69585a9 to
010e58d
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- README.noformat (6 hunks)
- src/maiken/setup.cpp (3 hunks)
- src/project.cpp (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- README.noformat
- src/maiken/setup.cpp
- src/project.cpp
Summary by CodeRabbit
New Features
subfield under thetestprofile in project configurations.Bug Fixes
MKN_DBGenvironment variable setting for gdb.Documentation
MKN_DEFAULT_BRANCHand updated descriptions for existing keys likeMKN_COMPILE_THREADSandMKN_DBG.Refactor
bin.cppfile related to building executables and libraries.modmethod inmods.cppfor improved parsing of project information.Style
#include <cstdint>in thegcc.cppfile for consistency.uint16_ttostd::uint16_tforllvvariable declaration ingcc.cpp.Chores
moduleparameter in theREPOandGETfunctions of theSCMGetterclass inscm.hpp.validatormethod's return type in theSettingsclass insettings.hppandsettings.cpp.validatormethod inproject.cppto include additional validation nodes like "sub" for project validation.