Skip to content

refactor: Update include statements to match our latest style guidelines (resolves #140). - #141

Merged
sitaowang1998 merged 12 commits into
y-scope:mainfrom
sitaowang1998:relative-include-path
May 22, 2025
Merged

refactor: Update include statements to match our latest style guidelines (resolves #140). #141
sitaowang1998 merged 12 commits into
y-scope:mainfrom
sitaowang1998:relative-include-path

Conversation

@sitaowang1998

@sitaowang1998 sitaowang1998 commented May 20, 2025

Copy link
Copy Markdown
Collaborator

Description

Use non-relative header include paths for src/spider and tests.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • GitHub workflows pass.

Summary by CodeRabbit

  • Refactor

    • Standardized all internal include paths across the codebase and tests to use fully qualified, project-root-relative paths (e.g., spider/core/...), replacing previous relative paths for improved clarity and consistency.
    • Updated build configuration files to ensure correct include directories are set for both source and test targets.
    • Enhanced code formatting rules to better distinguish project relative headers from project library headers.
  • Chores

    • No changes to functionality or public interfaces; all updates are internal refactoring for maintainability.

@sitaowang1998
sitaowang1998 requested a review from a team as a code owner May 20, 2025 16:03
@coderabbitai

coderabbitai Bot commented May 20, 2025

Copy link
Copy Markdown
Contributor

"""

Walkthrough

This change systematically replaces all relative include paths in source and test files with project-root-based absolute include paths prefixed by spider/ or tests/. Additionally, CMake configuration files are updated to use target_include_directories instead of exposing header files via target_sources. The .clang-format configurations are also refined to better categorize project and third-party headers. No functional code or logic is altered.

Changes

File(s) Change Summary
src/spider/CMakeLists.txt, tests/CMakeLists.txt Switched to using target_include_directories for include paths in CMake; removed target_sources for headers.
src/spider/client/, src/spider/core/, src/spider/io/, src/spider/scheduler/, src/spider/storage/, src/spider/worker/ Replaced all relative #include paths with project-root-based absolute includes prefixed by spider/.
tests/client/, tests/io/, tests/scheduler/, tests/storage/, tests/utils/, tests/worker/ Refactored test and utility includes to use project-root-based include paths, e.g., spider/ or tests/.
src/spider/.clang-format, examples/quick-start/.clang-format, tests/.clang-format Updated .clang-format configurations to add distinct categories for project library headers and project relative headers, improving header grouping and priority.

Possibly related PRs

Suggested reviewers

  • davidlion
    """

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3443d14 and df69302.

📒 Files selected for processing (3)
  • examples/quick-start/.clang-format (1 hunks)
  • src/spider/.clang-format (2 hunks)
  • tests/.clang-format (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/spider/.clang-format
  • examples/quick-start/.clang-format
  • tests/.clang-format
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: lint
  • GitHub Check: non-storage-unit-tests (ubuntu-24.04)
  • GitHub Check: non-storage-unit-tests (ubuntu-22.04)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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 using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

anlowee
anlowee previously approved these changes May 20, 2025
@sitaowang1998
sitaowang1998 requested a review from davidlion May 20, 2025 16:09

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
src/spider/io/msgpack_message.hpp (1)

1-2: ⚠️ Potential issue

Include guard macro does not match file path
The header is located in io, but the guard is named SPIDER_CORE_MSGPACKMESSAGE_HPP, which risks collisions and reduces clarity.
Apply this diff to align the macro with its directory:

-#ifndef SPIDER_CORE_MSGPACKMESSAGE_HPP
-#define SPIDER_CORE_MSGPACKMESSAGE_HPP
+#ifndef SPIDER_IO_MSGPACKMESSAGE_HPP
+#define SPIDER_IO_MSGPACKMESSAGE_HPP
...
-#endif  // SPIDER_CORE_MSGPACKMESSAGE_HPP
+#endif  // SPIDER_IO_MSGPACKMESSAGE_HPP

Also applies to: 24-24

🧹 Nitpick comments (6)
src/spider/io/msgpack_message.hpp (1)

7-8: Header include paths updated correctly; unify IWYU pragma formatting
The absolute includes for BoostAsio and MsgPack are correct . However, the IWYU pragmas are inconsistent (// IWYU pragma: keep vs // IWYU pragma :keep). Please normalise both to the standard // IWYU pragma: keep.

tests/CMakeLists.txt (3)

22-22: Ensure test target include directory resolves project headers correctly

The test target uses:

target_include_directories(unitTest PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/..)

which points at the repo root. Since your headers live under src/spider/... and your code now does #include "spider/...", you likely need to add the src directory explicitly so the <spider/...> includes resolve. Consider:

-target_include_directories(unitTest PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/..)
+target_include_directories(unitTest PRIVATE
+    ${CMAKE_CURRENT_SOURCE_DIR}/../src
+)

58-58: Restrict include directory visibility and update path

Currently:

target_include_directories(worker_test PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/..)

PUBLIC propagates this include dir to dependents unnecessarily, and it still points at the repo root. Prefer:

-target_include_directories(worker_test PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/..)
+target_include_directories(worker_test PRIVATE
+    ${CMAKE_CURRENT_SOURCE_DIR}/../src
+)

to lock it down and ensure "spider/..." headers are found.


82-82: Verify signal_test header path

The signal_test target now has:

target_include_directories(signal_test PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/..)

but headers live under src/spider. You may want:

-target_include_directories(signal_test PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/..)
+target_include_directories(signal_test PRIVATE
+    ${CMAKE_CURRENT_SOURCE_DIR}/../src
+)

to match your new include convention.

src/spider/client/Driver.cpp (1)

15-21: Unify include style for all headers

Internal headers have been standardized to:

#include "spider/..."

but the local header is still:

#include "Driver.hpp"

For consistency, update it to:

-#include "Driver.hpp"
+#include "spider/client/Driver.hpp"
src/spider/client/TaskContext.cpp (1)

12-15: Consistent header includes

The refactor changes external includes to "spider/...", yet this file still does:

#include "TaskContext.hpp"

Please switch to:

-#include "TaskContext.hpp"
+#include "spider/client/TaskContext.hpp"

to keep the include style uniform.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9a29306 and ea54ccf.

📒 Files selected for processing (65)
  • src/spider/CMakeLists.txt (1 hunks)
  • src/spider/client/Data.hpp (1 hunks)
  • src/spider/client/Driver.cpp (1 hunks)
  • src/spider/client/Driver.hpp (1 hunks)
  • src/spider/client/Job.hpp (1 hunks)
  • src/spider/client/TaskContext.cpp (1 hunks)
  • src/spider/client/TaskContext.hpp (1 hunks)
  • src/spider/client/TaskGraph.hpp (1 hunks)
  • src/spider/client/spider.hpp (1 hunks)
  • src/spider/client/task.hpp (1 hunks)
  • src/spider/core/DataImpl.hpp (1 hunks)
  • src/spider/core/Task.cpp (1 hunks)
  • src/spider/core/Task.hpp (1 hunks)
  • src/spider/core/TaskContextImpl.hpp (1 hunks)
  • src/spider/core/TaskGraph.hpp (1 hunks)
  • src/spider/core/TaskGraphImpl.hpp (1 hunks)
  • src/spider/io/Serializer.hpp (1 hunks)
  • src/spider/io/msgpack_message.cpp (1 hunks)
  • src/spider/io/msgpack_message.hpp (1 hunks)
  • src/spider/scheduler/FifoPolicy.cpp (1 hunks)
  • src/spider/scheduler/FifoPolicy.hpp (1 hunks)
  • src/spider/scheduler/SchedulerMessage.hpp (1 hunks)
  • src/spider/scheduler/SchedulerServer.cpp (1 hunks)
  • src/spider/scheduler/SchedulerServer.hpp (1 hunks)
  • src/spider/scheduler/scheduler.cpp (1 hunks)
  • src/spider/storage/DataStorage.hpp (1 hunks)
  • src/spider/storage/JobSubmissionBatch.hpp (1 hunks)
  • src/spider/storage/MetadataStorage.hpp (1 hunks)
  • src/spider/storage/StorageFactory.hpp (1 hunks)
  • src/spider/storage/mysql/MySqlConnection.cpp (1 hunks)
  • src/spider/storage/mysql/MySqlConnection.hpp (1 hunks)
  • src/spider/storage/mysql/MySqlJobSubmissionBatch.cpp (1 hunks)
  • src/spider/storage/mysql/MySqlJobSubmissionBatch.hpp (1 hunks)
  • src/spider/storage/mysql/MySqlStorage.cpp (1 hunks)
  • src/spider/storage/mysql/MySqlStorage.hpp (1 hunks)
  • src/spider/storage/mysql/MySqlStorageFactory.cpp (1 hunks)
  • src/spider/storage/mysql/MySqlStorageFactory.hpp (1 hunks)
  • src/spider/worker/DllLoader.cpp (1 hunks)
  • src/spider/worker/FunctionManager.cpp (1 hunks)
  • src/spider/worker/FunctionManager.hpp (1 hunks)
  • src/spider/worker/TaskExecutor.cpp (1 hunks)
  • src/spider/worker/TaskExecutor.hpp (1 hunks)
  • src/spider/worker/TaskExecutorMessage.hpp (1 hunks)
  • src/spider/worker/WorkerClient.cpp (1 hunks)
  • src/spider/worker/WorkerClient.hpp (1 hunks)
  • src/spider/worker/message_pipe.cpp (1 hunks)
  • src/spider/worker/message_pipe.hpp (1 hunks)
  • src/spider/worker/task_executor.cpp (1 hunks)
  • src/spider/worker/worker.cpp (1 hunks)
  • tests/CMakeLists.txt (3 hunks)
  • tests/client/client-test.cpp (1 hunks)
  • tests/client/test-Driver.cpp (1 hunks)
  • tests/io/test-MsgpackMessage.cpp (1 hunks)
  • tests/scheduler/test-SchedulerPolicy.cpp (1 hunks)
  • tests/scheduler/test-SchedulerServer.cpp (1 hunks)
  • tests/storage/StorageTestHelper.hpp (1 hunks)
  • tests/storage/test-DataStorage.cpp (1 hunks)
  • tests/storage/test-MetadataStorage.cpp (1 hunks)
  • tests/utils/CoreDataUtils.hpp (1 hunks)
  • tests/utils/CoreTaskUtils.cpp (1 hunks)
  • tests/utils/CoreTaskUtils.hpp (1 hunks)
  • tests/worker/test-FunctionManager.cpp (1 hunks)
  • tests/worker/test-MessagePipe.cpp (1 hunks)
  • tests/worker/test-Process.cpp (1 hunks)
  • tests/worker/test-TaskExecutor.cpp (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: lint
  • GitHub Check: non-storage-unit-tests (ubuntu-24.04)
  • GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (62)
src/spider/storage/JobSubmissionBatch.hpp (1)

4-5: Use project-root-relative include paths

The includes have been updated from relative paths to use the spider/... prefix, matching the project’s convention and CMake target_include_directories setup. This improves readability and maintainability without altering functionality.

src/spider/core/TaskContextImpl.hpp (1)

8-11: Consistent absolute include paths
The updated directives now correctly use spider/... paths and include all required headers (TaskContext, DataStorage, MetadataStorage, StorageFactory) without altering functionality.

src/spider/storage/StorageFactory.hpp (1)

7-11: Standardized storage factory include paths
The includes for Error, DataStorage, JobSubmissionBatch, MetadataStorage, and StorageConnection have been updated to project-root-based spider/... paths, aligning with the refactor and preserving existing interfaces.

src/spider/storage/DataStorage.hpp (1)

8-11: Unified include paths for DataStorage
All internal dependencies (Data, Error, KeyValueData, StorageConnection) are now consistently referenced using spider/... absolute paths, maintaining the class interface and behavior.

src/spider/client/TaskContext.hpp (1)

18-28: Project-root include paths in TaskContext.hpp
The header now references all client and core dependencies (Data, Exception, Job, task, TaskGraph, Error, TaskGraphImpl, Serializer, StorageConnection, StorageFactory) via spider/... paths, ensuring consistency without functional changes.

src/spider/client/Data.hpp (1)

14-20: Consistent include path updates
The Data template now imports necessary components (Exception, Error, MsgPack, Serializer, DataStorage, StorageConnection, StorageFactory) using uniform spider/... includes, preserving all existing functionality.

src/spider/CMakeLists.txt (1)

46-46: Expose include directory for headers
Adding target_include_directories(spider_core PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/..) correctly exposes the root include path (spider/…) for consumers of spider_core, aligning with the refactored non-relative includes.

src/spider/storage/MetadataStorage.hpp (1)

9-15: Consistent absolute include paths
The updated includes from "spider/core/Driver.hpp" through "spider/storage/StorageConnection.hpp" match the new project-root-relative style and maintain clear separation between core and storage headers. This aligns perfectly with the PR’s goal of removing relative paths.

tests/utils/CoreDataUtils.hpp (1)

3-3: Use project-root include for Data.hpp
Switching to "spider/core/Data.hpp" ensures the test util follows the new include convention and remains consistent with production code.

src/spider/core/TaskGraph.hpp (1)

15-15: Standardize include path for Task.hpp
Updating to "spider/core/Task.hpp" matches the absolute include pattern used elsewhere and removes any ambiguity in header resolution.

src/spider/client/TaskGraph.hpp (1)

7-7: Adopt absolute include for client/task.hpp
Changing the include to "spider/client/task.hpp" aligns this header with the project-root style and guarantees TaskIo and related templates are resolved consistently.

src/spider/worker/TaskExecutorMessage.hpp (1)

6-6: Convert include to project-root-relative path
The include directive has been updated from a relative path to "spider/io/MsgPack.hpp", aligning with the repository’s convention for non-relative includes.

src/spider/io/Serializer.hpp (1)

9-9: Standardise include path for MsgPack.hpp
Switching to "spider/io/MsgPack.hpp" ensures consistency with the absolute include strategy and honours the IWYU pragma.

src/spider/core/Task.cpp (1)

12-13: Update include directives to use spider/ prefix
Both MsgPack and Serializer headers now use fully qualified paths, matching the new include policy without altering any logic.

src/spider/core/TaskGraphImpl.hpp (1)

14-21: Migrate multiple includes to absolute, project-root-relative paths
All core, client and IO headers are now imported via spider/..., preserving IWYU pragmas and simplifying include resolution.

src/spider/worker/FunctionManager.hpp (1)

20-30: Refactor includes to use spider/-prefixed paths
Internal headers across client, core, IO, storage and worker modules have been updated consistently, facilitating clearer dependency management.

src/spider/scheduler/SchedulerMessage.hpp (1)

10-11: Include paths refactored to absolute project-root style
The relative imports for MsgPack and Serializer have been correctly replaced with spider/io/..., improving consistency across the codebase.

src/spider/worker/message_pipe.hpp (1)

7-8: Include paths updated to project-root style
The switch from ../io/... to spider/io/... for BoostAsio and MsgPack is accurate and aligns with the PR objective.

src/spider/worker/message_pipe.cpp (1)

14-15: Include paths updated to project-root style
The absolute imports for BoostAsio and MsgPack now use the spider/io/... prefix as intended.

tests/utils/CoreTaskUtils.cpp (1)

16-17: Test utilities updated to use absolute includes
The headers Task.hpp and TaskGraph.hpp have been correctly referenced via spider/core/..., aligning with the rest of the suite.

src/spider/core/Task.hpp (1)

16-17: Standardize include directives to project-root-relative paths
The two headers have been correctly updated from relative to absolute paths (spider/core/Data.hpp and spider/io/MsgPack.hpp), aligning with the new convention.

src/spider/storage/mysql/MySqlConnection.cpp (1)

15-16: Apply absolute include paths for core and storage headers
The includes for spider/core/Error.hpp and spider/storage/StorageConnection.hpp are correctly converted to project-root-based paths, matching the refactoring across the codebase.

src/spider/io/msgpack_message.cpp (1)

17-18: Switch to absolute includes for BoostAsio and MsgPack
The local includes have been updated to "spider/io/BoostAsio.hpp" and "spider/io/MsgPack.hpp" with IWYU pragmas retained—this change is consistent with the new include strategy.

src/spider/scheduler/scheduler.cpp (1)

23-35: Refactor scheduler includes to project-root-relative style
All dependencies under spider/core, spider/io, spider/scheduler, spider/storage, and spider/utils have been updated to absolute paths. This block is consistent with the rest of the refactor and preserves include grouping.

src/spider/client/Driver.hpp (2)

11-11: Add <utility> for move semantics
The addition of <utility> ensures std::move and related utilities are available, which is required by the class constructors invoking std::move.


18-31: Convert client-driver includes to absolute paths
All client, core, io, storage, and worker headers are now included via fully qualified spider/... paths. This aligns with project guidelines and maintains consistency.

tests/storage/StorageTestHelper.hpp (1)

10-11: Include path update is correct and consistent.
The switch to spider/storage/mysql/MySqlStorageFactory.hpp and spider/storage/StorageFactory.hpp aligns with the new project-root include convention and improves clarity.

src/spider/worker/task_executor.cpp (1)

23-33: Switch to non-relative include directives.
These updated includes consistently reference the spider/ namespace and preserve existing IWYU pragmas, matching the PR’s objective to standardize header paths.

tests/utils/CoreTaskUtils.hpp (1)

4-5: Standardize to project-root include paths.
Replacing the relative paths with spider/core/Task.hpp and spider/core/TaskGraph.hpp improves maintainability and aligns with the repository’s conventions.

src/spider/scheduler/FifoPolicy.hpp (1)

11-15: Convert to fully qualified include paths.
The new spider/core/Task.hpp, spider/scheduler/SchedulerPolicy.hpp, spider/storage/DataStorage.hpp, spider/storage/MetadataStorage.hpp, and spider/storage/StorageConnection.hpp includes adhere to the project-root convention and enhance readability.

src/spider/core/DataImpl.hpp (1)

7-9: Adopt project-root include directives.
Updating to spider/client/Data.hpp, spider/core/Data.hpp, and spider/storage/StorageFactory.hpp maintains consistency across modules and supports the updated CMake include settings.

src/spider/worker/DllLoader.cpp (1)

11-12: Consistent project-root include paths
Updated include directives correctly reference spider/worker headers, aligning with the project’s non-relative include style.

src/spider/worker/worker.cpp (1)

36-52: Standardize include directives
All project headers are now included with spider/... prefixes and IWYU pragmas are preserved, improving clarity and maintainability.

src/spider/storage/mysql/MySqlConnection.hpp (1)

11-12: Use absolute include paths for core and storage
The includes for spider/core/Error.hpp and spider/storage/StorageConnection.hpp correctly replace relative paths and maintain consistency with the project’s include conventions.

src/spider/storage/mysql/MySqlStorageFactory.cpp (1)

8-16: Switch to project-root includes for storage factory
Updated include directives to non-relative paths for core error, storage interfaces, and MySQL-specific headers, matching the refactoring across the storage module.

tests/io/test-MsgpackMessage.cpp (1)

13-15: Align test includes with project structure
Replaced relative test includes with spider/io/... paths and preserved IWYU pragmas, ensuring tests compile against the new header layout.

src/spider/scheduler/FifoPolicy.cpp (1)

13-16: Approve include path modernization

The updated include directives correctly use the project-root spider/... style without altering any semantics. CI confirms the build remains successful.

src/spider/scheduler/SchedulerServer.cpp (1)

14-24: Approve include path updates

The switch to spider/... include paths is consistent and does not affect functionality. All existing IWYU pragmas remain in place and CI passes.

src/spider/worker/WorkerClient.cpp (1)

18-28: Approve absolute include path standardization

The updated includes follow the spider/... convention and preserve the necessary IWYU pragmas. No behavioural changes introduced.

src/spider/storage/mysql/MySqlJobSubmissionBatch.hpp (1)

8-10: Approve modernization of include directives

The include paths were converted to use the spider/... prefix correctly, aligning with project style. Class definitions and logic remain untouched.

src/spider/client/task.hpp (1)

6-8: Approve refactored include paths

Headers now use root-based spider/... includes per project convention. All template concepts and declarations are unchanged.

src/spider/worker/WorkerClient.hpp (1)

11-14: Use absolute include paths consistently
The refactored include directives now uniformly reference the spider/ root, improving clarity and maintainability. The IWYU pragmas are preserved correctly.

src/spider/client/spider.hpp (1)

5-9: Standardise client module include paths
The change to fully qualified spider/client/... headers enhances discoverability and aligns with the project’s include path conventions.

src/spider/storage/mysql/MySqlJobSubmissionBatch.cpp (1)

7-10: Align MySQL storage includes with project root
Updating these headers to use spider/... paths maintains consistency across storage modules without altering functionality.

src/spider/worker/FunctionManager.cpp (1)

14-15: Update worker includes to project-root paths
The modification to spider/io/MsgPack.hpp and spider/worker/TaskExecutorMessage.hpp follows the new include path standard and keeps the IWYU directives intact.

src/spider/storage/mysql/MySqlStorageFactory.hpp (1)

8-13: Switch StorageFactory includes to absolute paths
These headers now correctly reference the spider/ prefix, which improves readability and reduces dependency on relative paths.

tests/scheduler/test-SchedulerServer.cpp (1)

15-29: Excellent standardization of include paths.

The change from relative include paths to project-root-based absolute paths improves maintainability and readability. This standardization makes it easier to relocate files within the project structure without having to update numerous include paths.

These non-relative paths make the code more robust against refactoring and provide clearer visibility into the project's structure.

src/spider/storage/mysql/MySqlStorage.hpp (1)

13-25: Great improvement to include directives.

Converting from relative paths (which likely involved multiple levels of "../") to absolute paths with the "spider/" prefix significantly enhances code readability and maintainability. This is especially valuable in deeply nested directories like this MySQL storage implementation.

The change eliminates the need to count directory levels when adding new includes and makes the code structure immediately obvious to developers.

tests/worker/test-Process.cpp (1)

10-11: Consistent include path standardization.

The change to project-root-based paths aligns with the codebase-wide standardization and improves maintainability. The IWYU pragma is properly preserved.

src/spider/scheduler/SchedulerServer.hpp (1)

8-12: Good include path standardization.

Converting to project-root-based include paths improves maintainability and clarity. This approach makes it instantly clear which project component each header belongs to and eliminates complex relative path calculations.

The IWYU pragma comment is properly preserved, maintaining build optimization directives.

tests/worker/test-FunctionManager.cpp (1)

14-28: Refactored include paths look correct
The updated headers now use project-root-based spider/ and tests/ prefixes, replacing the previous relative paths. This enhances clarity and maintainability without altering behaviour.

tests/worker/test-MessagePipe.cpp (1)

9-13: Absolute include paths applied successfully
The change to #include "spider/io/BoostAsio.hpp" and other internal headers aligns with the project standard. No logic or test behaviour is affected.

src/spider/worker/TaskExecutor.hpp (1)

24-28: Updated internal includes to use spider/ prefix
Switching from relative to absolute includes here is consistent with the rest of the codebase. Ensure that the include directories in CMake are correctly set to resolve these paths.

src/spider/worker/TaskExecutor.cpp (1)

13-18: Project include directives standardised
The move to absolute spider/ includes for BoostAsio, MsgPack, FunctionManager, etc., is purely stylistic and maintains existing functionality.

tests/scheduler/test-SchedulerPolicy.cpp (1)

15-25: Scheduler test includes aligned with new style
Replacing relative includes with spider/ and tests/ prefixes keeps the test code consistent and clear. No behavioural changes detected.

tests/worker/test-TaskExecutor.cpp (1)

19-32: Absolute include paths look correct

The include directives have been updated from relative to absolute ("spider/..." and "tests/...") and align with the new project layout. Everything here looks good.

tests/client/test-Driver.cpp (1)

9-13: Project and test headers updated properly

Your #include directives now use project-root paths ("spider/..." and "tests/...") matching the refactored structure. LGTM.

tests/client/client-test.cpp (1)

15-19:

Details

✅ Verification successful

Consistent non‐relative includes
The include directives have been updated to use project‐root‐based paths (spider/… and tests/…), improving clarity and maintainability.

Run the following to ensure your CMakeLists setup covers both source and test roots:


🏁 Script executed:

#!/bin/bash
# Search for include setup in CMakeLists.txt
rg "target_include_directories" -n .

Length of output: 568


CMake include paths already cover project-root-based includes
Both src/spider/CMakeLists.txt (lines 46, 179) and tests/CMakeLists.txt (lines 22, 58, 81) add ${CMAKE_CURRENT_SOURCE_DIR}/.. to their include paths, so the spider/... and tests/... headers in tests/client/client-test.cpp resolve correctly. No further changes required.

tests/storage/test-MetadataStorage.cpp (1)

16-26: Localized include path standardization approved
Updated headers now consistently reference spider/… and tests/… paths, aligning with project conventions and requiring no further adjustments.

src/spider/client/Job.hpp (1)

19-29:

Details

✅ Verification successful

Root‐based header includes applied
Headers have been converted from relative to absolute paths. Confirm that all listed headers (notably spider/client/task.hpp and spider/client/type_utils.hpp) exist with correct casing, and verify that src/spider is included in your build’s include directories.

Run this script to verify header file existence:


🏁 Script executed:

#!/bin/bash
# Check for the existence of each updated include
for hdr in Data.hpp Exception.hpp task.hpp type_utils.hpp; do
  echo -n "Checking spider/client/$hdr: "
  [ -f src/spider/client/$hdr ] && echo "Found" || echo "Missing"
done

Length of output: 824


🏁 Script executed:

#!/usr/bin/env bash
# Search for CMake include directives and compiler flags referencing src/spider
echo "Scanning for include_directories in CMakeLists:"
rg "include_directories" -n .
rg "target_include_directories" -n .
echo "Scanning for -I flags pointing to src/spider:"
rg "-I[^ ]*src/spider" -n .

Length of output: 1771


Root‐based includes verification complete
All specified headers exist under src/spider/client, and the project’s CMakeLists (src/spider/CMakeLists.txt) correctly adds ${CMAKE_CURRENT_SOURCE_DIR}/.. to the include path, covering src/spider. No further action needed.

tests/storage/test-DataStorage.cpp (1)

11-22: Approved: standardized includes
Include directives now uniformly use spider/… and tests/… paths. This enhances consistency and readability without affecting test logic.

src/spider/storage/mysql/MySqlStorage.cpp (1)

31-43: Approved: MySQL storage headers updated
Includes have been refactored to use project‐root‐based paths for core, storage, and MySQL‐specific headers. Ensure that both src/spider and src/spider/storage/mysql directories are added to your build’s include paths.

@davidlion davidlion changed the title fix: Use non-relative header include paths (fixes #140). refactor: Update include statements to match our latest style guidelines (resolves #140). May 21, 2025

@davidlion davidlion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed let's change the project library header include statements to use <> rather than "".

Comment thread src/spider/.clang-format
@sitaowang1998
sitaowang1998 requested a review from davidlion May 22, 2025 02:31
Comment thread src/spider/.clang-format Outdated
Co-authored-by: davidlion <davidlion2@protonmail.com>
Comment thread tests/.clang-format
Co-authored-by: davidlion <davidlion2@protonmail.com>
Comment thread src/spider/.clang-format Outdated
Comment on lines 4 to 8
# NOTE: A header is grouped by first matching regex
# Project headers
# Project library headers
- Regex: "^<spider"
Priority: 4
# Library headers. Update when adding new libraries.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I just realized all of the .clang-format files have a weird issue where the project library headers split up the comment. Unfortunately, I can't make the full suggestion.

It should be:

  # NOTE: A header is grouped by first matching regex library headers. Update when adding new
  # libraries.
  # NOTE: clang-format retains leading white-space on a line in violation of the YAML spec.
  # Project library headers
  - Regex: "^<spider"
    Priority: 4
  # External library headers
  - Regex: "^<(absl|boost|catch2|fmt|mariadb|msgpack|spdlog|ystdlib)"
    Priority: 3
  # C system headers

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the "Update when adding new libraries" is a comment for external library headers only. Thus I move it there.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Thanks!

@sitaowang1998
sitaowang1998 merged commit 5388add into y-scope:main May 22, 2025
@sitaowang1998
sitaowang1998 deleted the relative-include-path branch May 22, 2025 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants