Fix(include_relative): Correctly resolve relative include paths#160
Fix(include_relative): Correctly resolve relative include paths#160michael-projectx merged 6 commits intomainfrom
Conversation
The include_relative directive was not correctly resolving paths relative to the current file. This change updates the logic in `base_include_action::apply` to use the parent directory of the current file (`in.position().source`) as the base for relative includes. If the input is from a string (i.e., `in.position().source` is empty), it falls back to `out.base_dir`. This commit also includes the patch from issue #159 to make `parseCommon` throw an exception on parsing failure, aiding in debugging and surfacing such issues. Additionally, `examples/config_example13.cfg` was updated to use correct paths for its includes, and the test environment now ensures `TEST_DIR` is properly set.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @miker2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request. This PR primarily addresses an issue where the include_relative directive was not correctly resolving paths relative to the file containing the directive. The fix involves updating the base_include_action logic to use the parent directory of the current source file as the base for relative includes. A fallback to the output base directory is included for cases where the source isn't a file path (like string inputs). Additionally, this PR incorporates a change from issue #159, modifying the parseCommon function to throw an InvalidConfigException upon parsing failure, improving error handling and debugging. An example configuration file (examples/config_example13.cfg) has been updated to use the correct relative paths, and the test environment setup is adjusted to ensure TEST_DIR is properly configured.
Highlights
- Improve parser error handling: The
parseCommonfunction insrc/config_parser.cpphas been updated to throw anInvalidConfigExceptionwhen parsing fails, instead of returning a boolean. This provides clearer error reporting and aligns with the change proposed in issue #159. - Update example configuration: The
examples/config_example13.cfgfile has been modified to use the correct relative paths for its includes, reflecting the fix to theinclude_relativedirective.
Changelog
Click here to see the changelog
- examples/config_example13.cfg
- Updated
includeandinclude_relativepaths to correctly reference files within theenvsubdirectory, likely to align with the corrected relative include logic or test setup.
- Updated
- include/flexi_cfg/config/actions.h
- Modified
base_include_action::applyto implement the correct resolution logic forinclude_relative. It now checks if the include is relative and, if so, attempts to use the parent directory of the input source file as the base path. Includes checks for valid source paths and falls back toout.base_dirif the source is not a usable file path. - Updated debug logging to include information about whether the include is relative, absolute, and the base path used for resolution.
- Modified
- src/config_parser.cpp
- Changed the
parseCommontemplate function to throw anflexi_cfg::config::InvalidConfigExceptionon parsing failure, rather than returning a boolean status. - Removed the boolean
successvariable and related return statements. - Updated the catch block for
peg::parse_errorto also throw theInvalidConfigException. - Changed the logging level for the partial output printout from
criticaltodebug. - Updated comments in
Parser::parseandParser::parseFromStringto reflect thatparseCommonnow throws exceptions on failure.
- Changed the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the problem of resolving relative include paths and significantly enhances the parser's error handling by introducing exceptions. The changes are well-documented, and the updated example configuration correctly reflects the fix. The improved debug logging is also a valuable addition. Overall, this is a solid improvement to the codebase.
Summary of Findings
- Code Clarity and Redundancy: In
include/flexi_cfg/config/actions.h, a condition for determining the base path for relative includes contains a redundant check (!current_file_source.parent_path().empty()) becausecurrent_file_source.has_parent_path()already implies this. Simplifying it would enhance code clarity. - Improved Error Handling: The change in
src/config_parser.cppforparseCommonto throw exceptions on parsing failure instead of returning a boolean is a significant improvement, making error propagation more robust and explicit. - Addressed TODOs: The PR successfully addresses previous TODO comments related to error handling in the parser.
Merge Readiness
This pull request introduces valuable improvements to path resolution and error handling. The code is generally in good shape. I've pointed out one area for potential simplification in include/flexi_cfg/config/actions.h which, if addressed, would further enhance code clarity. As I am an AI assistant, I am not authorized to approve pull requests. Please ensure further review and approval by team members before merging. Addressing the medium severity comment is recommended.
…ssue #159 This commit addresses issue #159 where the `include_relative` directive was not correctly resolving paths relative to the current file. Changes: - Modified `include/flexi_cfg/config/actions.h`: The `base_include_action::apply` method now correctly determines the base path for `include_relative` directives. If the current file's source is available, its parent directory is used. Otherwise (e.g., for string inputs), `out.base_dir` is used. This ensures that paths specified with `include_relative`, after environment variable substitution, are resolved from the correct base directory. - Applied patch to `src/config_parser.cpp`: The `parseCommon` function now throws an `InvalidConfigException` on parsing errors instead of returning a boolean. This aligns with the repro steps in the issue and improves error reporting. - Corrected `examples/config_example13.cfg`: The file has been reverted to its state from commit `f7cefaba1a07a0567073ac6eea0a820851ff903a`, which is the version referenced in the issue as demonstrating the bug. These changes ensure that `include_relative` works as intended, particularly in conjunction with environment variables like `TEST_DIR` (which is expected to be set to `env` for tests). All tests pass with this corrected logic and configuration.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…nested `include_relative` directives.
This commit enhances test coverage for the `include_relative` directive by introducing a nested include scenario.
- You've added `examples/env/env_example3.cfg` with distinct values.
- You've modified `examples/env/env_example2.cfg` to include `env_example3.cfg` using `include_relative env_example3.cfg`.
When `examples/config_example13.cfg` is parsed (which includes `${TEST_DIR}/env_example2.cfg` via `include_relative`), it now transitively includes `env_example3.cfg`.
This ensures that the `include_relative` path resolution logic correctly handles includes from files that are themselves included via a relative path. All tests continue to pass with this new nested include structure.
This commit refines the condition used to determine the base directory for `include_relative` directives in `include/flexi_cfg/config/actions.h`. The check `!current_file_source.parent_path().empty()` was redundant when `current_file_source.has_parent_path()` is already being checked. If `has_parent_path()` is true, `parent_path()` will not be empty. If `has_parent_path()` is false, `parent_path()` returns an empty path, so the explicit check for emptiness of the parent path is not needed in conjunction with `has_parent_path()`. This change simplifies the conditional logic without altering behavior, as confirmed by all tests passing.
This commit further refines the `base_include_action::apply` method in `include/flexi_cfg/config/actions.h`. The variable used to store the base path for include resolution (`path_base`) is initialized to `out.base_dir`. An `if` condition subsequently updates it to the current file's parent directory for `include_relative` if applicable. The `else` branch that re-assigned `path_base` to `out.base_dir` was redundant, as this was already its initial value. The redundant `else` has been removed, and a comment was added to clarify the default/fallback behavior. This change makes the code slightly cleaner without altering functionality, as confirmed by all tests passing.
* Fixes include_relative to correctly resolve relative include paths This commit addresses issue #159 where the `include_relative` directive was not correctly resolving paths relative to the current file. This commit also includes the patch from issue #159 to make `parseCommon` throw an exception on parsing failure, aiding in debugging and surfacing such issues. Changes: - Modified `include/flexi_cfg/config/actions.h`: The `base_include_action::apply` method now correctly determines the base path for `include_relative` directives. If the current file's source is available, its parent directory is used. Otherwise (e.g., for string inputs), `out.base_dir` is used. This ensures that paths specified with `include_relative` are resolved from the correct base directory. - Adds additional test coverage for usage of the `include_relative` syntax
2 participants
The include_relative directive was not correctly resolving paths relative to the current file. This change updates the logic in
base_include_action::applyto use the parent directory of the current file (in.position().source) as the base for relative includes.If the input is from a string (i.e.,
in.position().sourceis empty), it falls back toout.base_dir.This commit also includes the patch from issue #159 to make
parseCommonthrow an exception on parsing failure, aiding in debugging and surfacing such issues.Additionally,
examples/config_example13.cfgwas updated to use correct paths for its includes, and the test environment now ensuresTEST_DIRis properly set.