Skip to content

Fixing docqueries generator flag -level & updating developer tools #2723

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 19, 2025

Conversation

cvvergara
Copy link
Member

@cvvergara cvvergara commented Jan 18, 2025

  • Fixes flag -level on docqueries
  • Updates the run.sh and taptest.sh developers helper files

@pgRouting/admins

Summary by CodeRabbit

  • New Features

    • Enhanced developer script with new functions for static analysis and code tidying.
    • Improved database testing script with version-aware testing mechanism.
    • Updated documentation query generator with more flexible command-line options.
  • Bug Fixes

    • Corrected command-line argument parsing in documentation query generator.
    • Fixed variable assignment in testing scripts.
  • Chores

    • Refined script organization and functionality in developer tools.
    • Modularized database setup and testing processes.

@cvvergara cvvergara added the tools Modify a tool label Jan 18, 2025
@cvvergara cvvergara added this to the Release 3.7.2 milestone Jan 18, 2025
Copy link
Contributor

coderabbitai bot commented Jan 18, 2025

Walkthrough

The pull request introduces modifications to three scripts in the developer and testing tools directory. The run.sh script now includes new functions for static code analysis and build processes, with enhanced test and documentation generation workflows. The taptest.sh script has been updated to support more dynamic version-based database testing, introducing version extraction and modular setup. The doc_queries_generator.pl script saw improvements in command-line argument parsing, specifically for log level specification.

Changes

File Changes
tools/developer/run.sh - Added check function for CMake configuration and static analysis
- Added tidy_with_clang function for clang-tidy checks
- Modified set_compiler to export CC and CXX directly
- Updated build_doc to preserve custom files
- Refined test_compile control flow and echo statements
tools/developer/taptest.sh - Added version extraction from CMakeLists.txt
- Introduced default values for PGPORT and PGUSER
- Replaced multiple psql commands with a call to setup_db.sh
- Added version-aware testing mechanism
tools/testers/doc_queries_generator.pl - Enhanced -l option parsing to support -l and -level
- Corrected $LEVEL variable assignment
- Added print statement for current logging level

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant RunScript as run.sh
    participant CMake as CMake
    participant Cppcheck as Cppcheck
    participant ClangTidy as Clang-Tidy
    
    Dev->>RunScript: Execute build process
    RunScript->>CMake: Generate compile commands
    CMake-->>RunScript: compile_commands.json
    RunScript->>Cppcheck: Static analysis
    RunScript->>ClangTidy: Code tidying
    RunScript->>RunScript: Build documentation
Loading

Poem

🐰 In the realm of code, where rabbits roam free,
Scripts dance and twirl with newfound glee
Static checks hop, versions take flight
Flexibility blooms, testing shines bright
A developer's warren of pure delight! 🔧


🪧 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.
    • Generate unit testing code for this file.
    • 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 generate unit testing code for this file.
    • @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 generate unit testing code.
    • @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.

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. (Beta)
  • @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.

@cvvergara cvvergara requested a review from robe2 January 18, 2025 19:05
@cvvergara
Copy link
Member Author

@coderabbitai configuration

Copy link
Contributor

coderabbitai bot commented Jan 18, 2025

CodeRabbit configuration
language: en-US
tone_instructions: ''
early_access: true
enable_free_tier: true
reviews:
  profile: assertive
  request_changes_workflow: false
  high_level_summary: true
  high_level_summary_placeholder: '@coderabbitai summary'
  auto_title_placeholder: '@coderabbitai'
  auto_title_instructions: ''
  review_status: true
  commit_status: true
  fail_commit_status: false
  collapse_walkthrough: false
  changed_files_summary: true
  sequence_diagrams: true
  assess_linked_issues: true
  related_issues: true
  related_prs: true
  suggested_labels: true
  auto_apply_labels: false
  suggested_reviewers: true
  poem: true
  labeling_instructions: []
  path_filters: []
  path_instructions: []
  abort_on_close: true
  auto_review:
    enabled: true
    auto_incremental_review: true
    ignore_title_keywords: []
    labels: []
    drafts: false
    base_branches: []
  tools:
    shellcheck:
      enabled: true
    ruff:
      enabled: true
    markdownlint:
      enabled: true
    github-checks:
      enabled: true
      timeout_ms: 90000
    languagetool:
      enabled: true
      enabled_only: false
      level: default
      enabled_rules: []
      disabled_rules: []
      enabled_categories: []
      disabled_categories: []
    biome:
      enabled: true
    hadolint:
      enabled: true
    swiftlint:
      enabled: true
    phpstan:
      enabled: true
      level: default
    golangci-lint:
      enabled: true
    yamllint:
      enabled: true
    gitleaks:
      enabled: true
    checkov:
      enabled: true
    detekt:
      enabled: true
    eslint:
      enabled: true
    rubocop:
      enabled: true
    buf:
      enabled: true
    regal:
      enabled: true
    actionlint:
      enabled: true
    pmd:
      enabled: true
    cppcheck:
      enabled: true
    semgrep:
      enabled: true
    circleci:
      enabled: true
    ast-grep:
      packages: []
      rule_dirs: []
      util_dirs: []
      essential_rules: true
chat:
  auto_reply: true
knowledge_base:
  opt_out: false
  learnings:
    scope: auto
  issues:
    scope: auto
  jira:
    project_keys: []
  linear:
    team_keys: []
  pull_requests:
    scope: auto

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 4a68cd3 and 44b3d3a.

📒 Files selected for processing (3)
  • tools/developer/run.sh (4 hunks)
  • tools/developer/taptest.sh (2 hunks)
  • tools/testers/doc_queries_generator.pl (1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
tools/developer/run.sh

[info] 114-114: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 114-114: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 128-128: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 128-128: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 129-129: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 130-130: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 131-131: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 131-131: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 135-135: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 136-136: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 179-179: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 180-180: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)

🪛 GitHub Actions: Check files
tools/developer/run.sh

[info] 92-92: Double quote PGPORT variable to prevent globbing and word splitting. Use: tools/release-scripts/get_signatures.sh -p "${PGPORT}"


[info] 98-98: Double quote PGPORT variable to prevent globbing and word splitting. Use: tools/testers/doc_queries_generator.pl -documentation -pgport "$PGPORT"


[info] 88-183: Multiple commands appear to be unreachable. Check usage or ignore if invoked indirectly. Affects various commands throughout the script including echo statements, build commands, and function calls.

⏰ Context from checks skipped due to timeout of 90000ms (16)
  • GitHub Check: Ubuntu Boost (86)
  • GitHub Check: Ubuntu Boost (84)
  • GitHub Check: Ubuntu Boost (83)
  • GitHub Check: Ubuntu Boost (80)
  • GitHub Check: Ubuntu Boost (79)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Ubuntu Boost (78)
  • GitHub Check: Ubuntu Boost (77)
  • GitHub Check: Ubuntu Boost (76)
  • GitHub Check: Ubuntu Boost (75)
  • GitHub Check: macos (macos-14)
  • GitHub Check: Ubuntu Boost (68)
  • GitHub Check: Ubuntu Boost (56)
  • GitHub Check: macos (macos-latest)
  • GitHub Check: Ubuntu clang
  • GitHub Check: Check queries
🔇 Additional comments (4)
tools/testers/doc_queries_generator.pl (1)

127-129: LGTM! Command-line interface improvement

The changes enhance usability by supporting both -l and -level options, with clear feedback about the selected level.

tools/developer/taptest.sh (1)

70-80: LGTM! Robust version management

The version-based testing logic is well-implemented, properly handling both old and new versions with appropriate update commands.

tools/developer/run.sh (2)

127-132: LGTM! Added static analysis integration

The new check function properly integrates cppcheck with CMake's compile commands database.

🧰 Tools
🪛 Shellcheck (0.10.0)

[info] 128-128: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 128-128: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 129-129: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 130-130: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 131-131: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


[info] 131-131: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

(SC2317)


161-162: 🛠️ Refactor suggestion

Fix shellcheck warnings about unquoted variables

Variables should be quoted to prevent word splitting and globbing issues.

-        tools/testers/doc_queries_generator.pl  -alg "docqueries/${d}" -level WARNING  -pgport "${PGPORT}"
+        tools/testers/doc_queries_generator.pl  -alg "docqueries/${d}" -level WARNING  -pgport "${PGPORT}"

Likely invalid or redundant comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

♻️ Duplicate comments (1)
tools/developer/taptest.sh (1)

29-31: ⚠️ Potential issue

Add error handling for git commands.

The git command to find repository root still needs error handling in case it's not run from within a git repository.

-DIR=$(git rev-parse --show-toplevel)
-pushd "${DIR}" > /dev/null || exit 1
+if ! DIR=$(git rev-parse --show-toplevel 2>/dev/null); then
+    echo "Error: Must be run from within the git repository" >&2
+    exit 1
+fi
+pushd "${DIR}" > /dev/null || exit 1
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 44b3d3a and f02de03.

📒 Files selected for processing (2)
  • tools/developer/run.sh (4 hunks)
  • tools/developer/taptest.sh (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
  • GitHub Check: Ubuntu Boost (86)
  • GitHub Check: Ubuntu Boost (84)
  • GitHub Check: Ubuntu Boost (83)
  • GitHub Check: Ubuntu Boost (80)
  • GitHub Check: Ubuntu Boost (79)
  • GitHub Check: Ubuntu Boost (78)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Ubuntu Boost (77)
  • GitHub Check: Ubuntu Boost (76)
  • GitHub Check: Ubuntu Boost (75)
  • GitHub Check: Ubuntu Boost (68)
  • GitHub Check: Ubuntu clang
  • GitHub Check: Ubuntu Boost (56)
  • GitHub Check: Check queries
🔇 Additional comments (4)
tools/developer/taptest.sh (2)

43-45: LGTM! Good improvement on database configuration.

The changes address the previous feedback by:

  1. Using a standard default port
  2. Using the current system user as default

56-58: Clarify the purpose of sleep delay.

The 3-second sleep seems arbitrary. If this is for database connection stability, consider:

  1. Making the delay configurable
  2. Adding a comment explaining its purpose
  3. Using a more reliable mechanism like connection retry
tools/developer/run.sh (2)

67-67: LGTM! Good addition of compile commands export option.

The addition of -DCMAKE_EXPORT_COMPILE_COMMANDS=ON is valuable as it enables static analysis tools to work with the codebase.


157-163: LGTM! Good improvement to documentation query logging.

The addition of -level WARNING provides better control over logging output.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🔭 Outside diff range comments (1)
tools/developer/taptest.sh (1)

Line range hint 51-52: Remove redundant PGRVERSION assignment.

The first PGRVERSION assignment is immediately overwritten by the second one, making it redundant and potentially confusing.

-PGRVERSION="3.6.1 3.6.0 3.5.1 3.5.0 3.2.0 3.1.3 3.0.6"
-PGRVERSION="3.7.2"
+PGRVERSION="3.7.2"  # List of versions to test
♻️ Duplicate comments (2)
tools/developer/taptest.sh (2)

39-39: ⚠️ Potential issue

Version extraction needs improvement.

The current version extraction method using Perl regex (grep -Po) remains fragile and system-dependent.

Please implement the previously suggested robust version extraction:

-VERSION=$(grep -Po '(?<=project\(PGROUTING VERSION )[^;]+' CMakeLists.txt)
+if ! VERSION=$(grep -E '^[[:space:]]*project\(PGROUTING[[:space:]]+VERSION[[:space:]]+([^[:space:];]+)' CMakeLists.txt | sed -E 's/.*VERSION[[:space:]]+([^[:space:];]+).*/\1/'); then
+    echo "Error: Failed to extract version from CMakeLists.txt" >&2
+    exit 1
+fi
+
+if [[ -z "${VERSION}" ]]; then
+    echo "Error: Version not found in CMakeLists.txt" >&2
+    exit 1
+fi

66-71: ⚠️ Potential issue

Add error handling for database setup.

The call to setup_db.sh still lacks error handling.

Please implement the previously suggested error handling:

-bash setup_db.sh "${PGPORT}" "${PGDATABASE}" "${PGUSER}" "${v}"
+if ! bash setup_db.sh "${PGPORT}" "${PGDATABASE}" "${PGUSER}" "${v}"; then
+    echo "Error: Database setup failed" >&2
+    exit 1
+fi
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between f02de03 and edbef8b.

📒 Files selected for processing (1)
  • tools/developer/taptest.sh (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Ubuntu Boost (86)
  • GitHub Check: Ubuntu Boost (84)
  • GitHub Check: Ubuntu Boost (83)
  • GitHub Check: Ubuntu Boost (80)
  • GitHub Check: Ubuntu Boost (79)
  • GitHub Check: Ubuntu Boost (78)
  • GitHub Check: Ubuntu Boost (77)
  • GitHub Check: Ubuntu Boost (76)
  • GitHub Check: Ubuntu Boost (75)
  • GitHub Check: Ubuntu Boost (68)
  • GitHub Check: Ubuntu Boost (56)
  • GitHub Check: macos (macos-14)
  • GitHub Check: Ubuntu clang
  • GitHub Check: macos (macos-latest)
  • GitHub Check: Check queries
🔇 Additional comments (2)
tools/developer/taptest.sh (2)

29-34: LGTM! Error handling for git commands implemented.

The repository root check has been properly implemented with error handling as previously suggested.


73-86: ⚠️ Potential issue

Improve version update logic.

The version update section needs several improvements:

  1. Add error handling for pg_prove
  2. Verify extension update success
  3. Improve comment clarity

Apply these changes:

 if [[ "${v}" != "${VERSION}" ]]
 then
-    # run tests on old version
-    pg_prove "$QUIET" --normalize --directives --recurse "${PGFLAGS}"  -d "${PGDATABASE}" "pgtap/${DIR}"
+    echo "Running tests on version ${v} before update..."
+    if ! pg_prove "$QUIET" --normalize --directives --recurse "${PGFLAGS}"  -d "${PGDATABASE}" "pgtap/${DIR}"; then
+        echo "Error: Tests failed on version ${v}" >&2
+        exit 1
+    fi

-    # update to this version
     echo "--------------------------"
-    echo " update version"
+    echo "Updating from version ${v} to ${VERSION}..."
     echo "--------------------------"
-    psql "${PGFLAGS}" -d "$PGDATABASE" -c "SET client_min_messages TO DEBUG3; ALTER EXTENSION pgrouting UPDATE TO '${VERSION}';"
+    if ! psql "${PGFLAGS}" -d "$PGDATABASE" -c "SET client_min_messages TO DEBUG3; ALTER EXTENSION pgrouting UPDATE TO '${VERSION}';"; then
+        echo "Error: Failed to update pgrouting extension to version ${VERSION}" >&2
+        exit 1
+    fi
+
+    # Verify update success
+    if ! psql "${PGFLAGS}" -q -t -d "$PGDATABASE" -c "SELECT extversion FROM pg_extension WHERE extname = 'pgrouting';" | grep -q "^${VERSION}$"; then
+        echo "Error: Extension version mismatch after update" >&2
+        exit 1
+    fi
 fi

Likely invalid or redundant comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between edbef8b and 48792a0.

📒 Files selected for processing (2)
  • tools/developer/run.sh (4 hunks)
  • tools/developer/taptest.sh (2 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
tools/developer/run.sh

[warning] 117-117: This action ignores everything before the -o. Use ( ) to group.

(SC2146)

🪛 GitHub Actions: Check files
tools/developer/run.sh

[warning] 117-117: Shell script warning: find command's -o operator is not properly grouped with ( ), causing it to ignore everything before -o

⏰ Context from checks skipped due to timeout of 90000ms (16)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Ubuntu Boost (86)
  • GitHub Check: Ubuntu Boost (84)
  • GitHub Check: Ubuntu Boost (83)
  • GitHub Check: Ubuntu Boost (80)
  • GitHub Check: Ubuntu Boost (79)
  • GitHub Check: Ubuntu Boost (78)
  • GitHub Check: Ubuntu Boost (77)
  • GitHub Check: Ubuntu Boost (76)
  • GitHub Check: Ubuntu Boost (75)
  • GitHub Check: macos (macos-14)
  • GitHub Check: Ubuntu Boost (68)
  • GitHub Check: Check queries
  • GitHub Check: Ubuntu clang
  • GitHub Check: macos (macos-latest)
  • GitHub Check: Ubuntu Boost (56)
🔇 Additional comments (6)
tools/developer/taptest.sh (4)

29-34: LGTM! Well-implemented repository root check.

The implementation correctly ensures the script runs from within the git repository with proper error handling.


39-47: LGTM! Robust version extraction implementation.

The version extraction is well-implemented with proper error handling and validation.


55-57: Make PGPORT configurable like PGUSER.

For consistency, PGPORT should be configurable with a fallback value:

-PGPORT="5432"
+PGPORT="${PGPORT:-5432}"

87-108: LGTM! Well-implemented version update logic.

The version update implementation includes:

  • Proper version comparison
  • Error handling for the update process
  • Verification of update success
tools/developer/run.sh (2)

108-109: LGTM! Safer compiler selection implementation.

Good improvement replacing sudo with environment variables for compiler selection.


128-142: LGTM! Well-implemented static analysis function.

The function includes proper error handling, report generation, and appropriate suppressions.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 48792a0 and c99aced.

📒 Files selected for processing (2)
  • tools/developer/run.sh (4 hunks)
  • tools/developer/taptest.sh (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Ubuntu Boost (86)
  • GitHub Check: Ubuntu Boost (84)
  • GitHub Check: Ubuntu Boost (83)
  • GitHub Check: Ubuntu Boost (80)
  • GitHub Check: Ubuntu Boost (79)
  • GitHub Check: Ubuntu Boost (78)
  • GitHub Check: Ubuntu Boost (77)
  • GitHub Check: Ubuntu Boost (76)
  • GitHub Check: Ubuntu Boost (75)
  • GitHub Check: Ubuntu clang
  • GitHub Check: Ubuntu Boost (68)
  • GitHub Check: Ubuntu Boost (56)
  • GitHub Check: macos (macos-14)
  • GitHub Check: macos (macos-latest)
🔇 Additional comments (11)
tools/developer/taptest.sh (5)

29-34: LGTM! Proper repository root validation.

The implementation correctly validates that the script is run from within the git repository with appropriate error handling.


55-57: LGTM! Good use of environment variables with defaults.

The implementation correctly uses parameter expansion to provide sensible defaults while allowing override through environment variables.


76-79: LGTM! Proper error handling in database setup.

The implementation correctly handles database setup failures with appropriate error messaging.


71-71: 🧹 Nitpick (assertive)

Document the purpose of the sleep command.

The sleep 3 command appears to be used for giving time to read messages. Consider adding a comment explaining its purpose.

+    # Give time to read the version and test information before proceeding
     sleep 3

Likely invalid or redundant comment.


39-47: 🧹 Nitpick (assertive)

Consider enhancing version extraction robustness.

While the current implementation handles basic error cases well, consider these improvements:

  1. Validate version format (e.g., semantic versioning)
  2. Handle multi-line project declarations
 if ! VERSION=$(grep -E '^[[:space:]]*project\(PGROUTING[[:space:]]+VERSION[[:space:]]+([^[:space:];]+)' CMakeLists.txt | sed -E 's/.*VERSION[[:space:]]+([^[:space:];]+).*/\1/'); then
     echo "Error: Failed to extract version from CMakeLists.txt" >&2
     exit 1
 fi

 if [[ -z "${VERSION}" ]]; then
     echo "Error: Version not found in CMakeLists.txt" >&2
     exit 1
+elif ! echo "${VERSION}" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?(\+[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?$'; then
+    echo "Error: Invalid version format in CMakeLists.txt: ${VERSION}" >&2
+    exit 1
 fi

Likely invalid or redundant comment.

tools/developer/run.sh (6)

110-111: LGTM! Safe compiler configuration.

The implementation correctly uses environment variables instead of sudo for compiler selection.


118-119: LGTM! Proper documentation cleanup.

The implementation correctly uses find with proper grouping to clean only generated files while preserving custom content.


146-152: LGTM! Well-structured clang-tidy function.

The implementation correctly handles base branch configuration and error cases.


192-194: ⚠️ Potential issue

Remove duplicate tap_test call.

The tap_test function is called twice in succession.

     tap_test
     tools/testers/doc_queries_generator.pl -pgport $PGPORT

     build_doc
-    tap_test
     action_tests

Likely invalid or redundant comment.


176-181: 🧹 Nitpick (assertive)

Document the purpose of different documentation query options.

Add comments explaining when to use each option:

+        # Generate documentation with full examples and explanations
         #tools/testers/doc_queries_generator.pl  -alg "docqueries/${d}" -documentation  -pgport "${PGPORT}"
+        # Run with increased verbosity to show potential issues
         #tools/testers/doc_queries_generator.pl  -alg "docqueries/${d}" -level WARNING  -pgport "${PGPORT}"
+        # Execute queries and compare results with expected output
         tools/testers/doc_queries_generator.pl  -alg "docqueries/${d}" -pgport "${PGPORT}"

Likely invalid or redundant comment.


130-144: 🧹 Nitpick (assertive)

Document and enhance static analysis configuration.

Consider these improvements:

  1. Document the purpose of CMAKE_EXPORT_COMPILE_COMMANDS
  2. Allow configuration via environment variables
 function check {
     pushd build > /dev/null || exit 1
+    # Generate compile_commands.json for static analysis tools
     cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=ON ..

     # Run with error handling and report generation
     cppcheck --project=compile_commands.json \
-        --enable=all \
-        --suppress=missingIncludeSystem \
+        --enable=${CPPCHECK_CHECKS:-all} \
+        --suppress=${CPPCHECK_SUPPRESS:-missingIncludeSystem} \
         --error-exitcode=1 \
         --output-file=cppcheck-report.txt 2>&1 || {
         echo "Static analysis failed. See build/cppcheck-report.txt for details"

Likely invalid or redundant comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between c99aced and 2fd57e2.

📒 Files selected for processing (1)
  • tools/developer/run.sh (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
  • GitHub Check: Ubuntu Boost (86)
  • GitHub Check: Ubuntu Boost (84)
  • GitHub Check: Ubuntu Boost (83)
  • GitHub Check: Ubuntu Boost (80)
  • GitHub Check: Ubuntu Boost (79)
  • GitHub Check: Ubuntu Boost (78)
  • GitHub Check: Ubuntu Boost (77)
  • GitHub Check: Ubuntu Boost (76)
  • GitHub Check: Ubuntu Boost (75)
  • GitHub Check: Ubuntu Boost (68)
  • GitHub Check: Ubuntu Boost (56)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Ubuntu clang
  • GitHub Check: Check queries
🔇 Additional comments (3)
tools/developer/run.sh (3)

110-111: LGTM! Good improvement in compiler selection.

The change to use environment variables instead of sudo is a safer and more portable approach.


118-119: LGTM! Improved documentation cleanup approach.

The targeted cleanup approach with proper find command syntax is better than removing all files.


172-181: 🧹 Nitpick (assertive)

Improve documentation of doc_queries_generator.pl modes.

The different modes need better documentation and could be made configurable.

     echo --------------------------------------------
     echo  Execute documentation queries
     echo --------------------------------------------
+    # DOC_QUERY_MODE controls the generator mode:
+    # - documentation: Generate full documentation
+    # - warning: Show only warnings
+    # - compare: Compare results (default)
+    local mode=${DOC_QUERY_MODE:-"compare"}
     for d in ${QUERIES_DIRS}
     do
-        # generate the documentation queries
-        #tools/testers/doc_queries_generator.pl  -alg "docqueries/${d}" -documentation  -pgport "${PGPORT}"
-        # Show warnings
-        #tools/testers/doc_queries_generator.pl  -alg "docqueries/${d}" -level WARNING  -pgport "${PGPORT}"
-        # Compare differences on results
-        tools/testers/doc_queries_generator.pl  -alg "docqueries/${d}" -pgport "${PGPORT}"
+        case "$mode" in
+            documentation)
+                tools/testers/doc_queries_generator.pl -alg "docqueries/${d}" -documentation -pgport "${PGPORT}"
+                ;;
+            warning)
+                tools/testers/doc_queries_generator.pl -alg "docqueries/${d}" -level WARNING -pgport "${PGPORT}"
+                ;;
+            compare)
+                tools/testers/doc_queries_generator.pl -alg "docqueries/${d}" -pgport "${PGPORT}"
+                ;;
+        esac

Likely invalid or redundant comment.

@cvvergara cvvergara merged commit c16f1b5 into pgRouting:main Jan 19, 2025
23 checks passed
@cvvergara cvvergara deleted the fix-doc_queries_generator branch January 19, 2025 13:38
cvvergara added a commit to cvvergara/pgrouting that referenced this pull request Jan 19, 2025
…gRouting#2723)

* Fixing docqueries generator flag -level & updating developer tools
cvvergara added a commit that referenced this pull request Jan 19, 2025
…2723) (#2724)

* Fixing docqueries generator flag -level & updating developer tools
This was referenced Jan 27, 2025
@coderabbitai coderabbitai bot mentioned this pull request Mar 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Modify a tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants