-
-
Notifications
You must be signed in to change notification settings - Fork 1
Creating the basic code for pgORpy #3
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
Conversation
WalkthroughThis change introduces the initial version (v0.0.0) of the pgORpy project. It establishes the foundational project structure, including build scripts, configuration files, documentation, localization templates, test scripts, and CI/CD workflows. It implements two version-related PostgreSQL functions: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PostgreSQL
participant Extension (pgORpy)
User->>PostgreSQL: CREATE EXTENSION pgorpy;
PostgreSQL->>Extension (pgORpy): Load extension
User->>PostgreSQL: SELECT por_version();
PostgreSQL->>Extension (pgORpy): Execute por_version()
Extension (pgORpy)-->>PostgreSQL: Return version string
User->>PostgreSQL: SELECT * FROM por_full_version();
PostgreSQL->>Extension (pgORpy): Execute por_full_version()
Extension (pgORpy)-->>PostgreSQL: Return version, or_tools
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were identified. All changes are aligned with the objectives of establishing the initial version, extension functions, build infrastructure, documentation, localization, and test setup. Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 75
🔭 Outside diff range comments (1)
or-tools-license (1)
2-203: Add missing OR‐Tools NOTICE and centralize third-party licensesWe didn’t find an upstream NOTICE file for OR-Tools alongside the Apache-2.0 text. To satisfy Apache-2.0 requirements and streamline compliance:
• Create and include the OR-Tools NOTICE file (copy upstream NOTICE) next to the Apache-2.0 license.
• Move thisor-tools-licenseinto a dedicated directory for third-party licenses (for example:
licenses/ortools/LICENSElicenses/ortools/NOTICEThis ensures all attributions and notices are preserved and easy to locate.
🧹 Nitpick comments (118)
tools/scripts/typos_whitelist.txt (1)
9-10: Deduplicate identical whitelist entriesLine duplicates are unnecessary and can slow lookups. Remove one of the identical lines.
- * The generated swig_type_info structures are assigned staticly to an initial - * The generated swig_type_info structures are assigned staticly to an initial + * The generated swig_type_info structures are assigned staticly to an initial.editorconfig (2)
27-29: *YAML: set indent_size and include .yamlYAML typically uses 2 spaces. Also include *.yaml alongside *.yml.
[*.yml] indent_style = space +indent_size = 2 + +[*.yaml] +indent_style = space +indent_size = 2
31-33: CMake section header: brace grouping compatibilitySome EditorConfig parsers don’t support brace grouping. Prefer separate sections to maximize compatibility.
-[{CMakeLists.txt,*.cmake}] -indent_size = 2 -indent_style = space +[CMakeLists.txt] +indent_size = 2 +indent_style = space + +[*.cmake] +indent_size = 2 +indent_style = spacegitattributes (2)
15-15: *Remove duplicate pattern for .queriesDuplicate entries increase maintenance overhead.
-*.queries text eol=lf
6-14: Add common text patterns to enforce LF consistentlyRecommend explicitly covering SQL, CMake, YAML, and PO files.
*.c text eol=lf *.h text eol=lf *.cpp text eol=lf *.hpp text eol=lf +*.sql text eol=lf +*.cmake text eol=lf +CMakeLists.txt text eol=lf *.rst text eol=lf *.queries text eol=lf *.dox text eol=lf *.sh text eol=lf *.md text eol=lf +*.yml text eol=lf +*.yaml text eol=lf +*.po text eol=lfCONTRIBUTING.md (3)
3-12: Tighten phrasing and fix a minor grammar issueSmall readability/grammar improvements.
-We are really glad you are reading this because we need volunteer developers to +We are glad you are reading this because we welcome volunteer developers to help with this project. @@ -We want you to work on things you're excited about. Drop a message and if -someone can assist you, will contact you back ASAP. +We want you to work on things you're excited about. Drop a message and, if +someone can assist you, they will contact you ASAP.
47-50: Prefer “larger changes” over “bigger changes”Minor style improvement.
-Write a clear log message for your commits. One-line messages are fine for small -changes, but bigger changes should have more information. +Write a clear log message for your commits. One-line messages are fine for small +changes, but larger changes should include more information.
18-30: Clarify third‑party license compatibility (Apache‑2.0 deps with GPLv2‑or‑later)Given the use of Apache‑2.0 dependencies (e.g., OR‑Tools), it helps to note that distribution will comply under the “or later” clause (GPLv3-compatible). Consider adding a one-paragraph note here about third‑party license handling and NOTICE preservation.
Also applies to: 33-44
doc/requirements.txt (1)
1-3: Clarify Sphinx lower bound and upper-bound the theme to avoid surprise breakages
- Confirm if excluding 4.0.0 is intentional. If not, switch to >=.
- Consider adding an upper bound for the theme for reproducibility.
-Sphinx>4.0.0,<7.0.0 +Sphinx>=4.0.0,<7.0.0 -sphinx-bootstrap-theme>=0.8.0 +sphinx-bootstrap-theme>=0.8.0,<1.0.0docqueries/version/full_version.pg (2)
1-2: Typo in headerUse “Copyright (c)” instead of “CopyRight(c)”.
--- a/docqueries/version/full_version.pg +++ b/docqueries/version/full_version.pg --- CopyRight(c) pgRouting developers +-- Copyright (c) pgRouting developers
5-5: Avoid SELECT * in docs to keep outputs stableExplicitly list columns so docs don’t silently change if the function adds columns later.
-SELECT * FROM por_full_version(); +SELECT version, or_tools FROM por_full_version();NEWS.md (1)
9-12: Fix markdownlint issues (MD039, MD036) in NEWS
- MD039: Remove spaces/newlines inside link text and keep it on one line.
- MD036: Use a heading instead of bold text for section title.
-To see all issues & pull requests closed by this release see the -[Git closed milestone for 0.0.0 -](https://github.com/pgRouting/pgorpy/issues?utf8=%E2%9C%93&q=milestone%3A%22Release%200.0.0%22) -on Github. +To see all issues & pull requests closed by this release see the +[Git closed milestone for 0.0.0](https://github.com/pgRouting/pgorpy/issues?utf8=%E2%9C%93&q=milestone%3A%22Release%200.0.0%22) +on GitHub.-**Version functions** +### Version functionsAlso applies to: 14-14
doc/forward.html (1)
1-15: Optional: Modernize to HTML5.Consider switching to HTML5 doctype and dropping XHTML constraints; both the JS and meta refresh will continue to work and markup becomes simpler.
locale/pot/release_notes.pot (2)
22-24: Fix “GitHub” capitalization in msgid source.“Github” should be “GitHub” to match proper branding. Since POT files are generated, please correct this in the source RST so the POT regenerates cleanly.
If needed, I can point to the exact RST where this string originates so you can update it there.
25-27: Typo: “Mayors” → “Majors”.Assuming release notes sections are “Majors / Minors,” please fix the source string so the POT regenerates with “Majors”.
locale/pot/por_full_version.pot (1)
37-39: Standardize “OR-Tools” branding.Use “OR-Tools” (capital O, R; hyphen; capital T) consistently.
Apply this diff to the POT for quick visibility (but preferably fix the RST source):
-Returns pgORpy version information and or-tools version used. +Returns pgORpy version information and OR-Tools version used. ... -or-tools -OR-tools version +OR-Tools +OR-Tools versionAlso applies to: 76-81
locale/en/LC_MESSAGES/sampledata.po (3)
2-2: Align version string to 0.0.0 across artifactsHeaders use "0.0" while the release targets 0.0.0. Recommend standardizing to "0.0.0" for consistency across docs and locale files.
-# Copyright (C) pgORpy Contributors - Version 0.0 +# Copyright (C) pgORpy Contributors - Version 0.0.0 @@ -"Project-Id-Version: pgORpy 0.0\n" +"Project-Id-Version: pgORpy 0.0.0\n"Also applies to: 9-9
6-6: Confirm fuzzy statusThe file is marked fuzzy. If English is intended to fall back to source strings, keeping fuzzy is fine. If you want these entries to be used as-is, clear fuzzy flags before release.
31-32: Capitalize PythonMessage uses "python" in lowercase. To keep terminology consistent, update the source doc string to "Python" and regenerate .po files; .po msgid should not be hand-edited.
Would you like me to propose the change in doc/general/sampledata.rst and refresh locales?
doc/general/functions.rst (2)
10-12: Remove stray ‘|’ linesBare pipe characters render as visible content in Sphinx. If you just need spacing, use blank lines instead.
-| - - + +
21-29: Verify toctree targets resolveEnsure por_version and por_full_version files are discoverable from this path. If they live under doc/version/, use relative includes or ensure CMake/Sphinx adds the directory to rst include paths.
I can adjust paths to ../version/por_version and ../version/por_full_version if needed.
sql/version/full_version.sql (1)
3-3: Confirm copyright holder namingHeader says "pgRouting developers". For consistency with this repo’s branding, consider "pgORpy developers" unless the legal entity is intentionally pgRouting.
locale/en/LC_MESSAGES/functions.po (2)
2-2: Align version string to 0.0.0 across artifactsStandardize "0.0" to "0.0.0" in headers for consistency.
-# Copyright (C) pgORpy Contributors - Version 0.0 +# Copyright (C) pgORpy Contributors - Version 0.0.0 @@ -"Project-Id-Version: pgORpy 0.0\n" +"Project-Id-Version: pgORpy 0.0.0\n"Also applies to: 9-9
6-6: Confirm fuzzy statusIf these English entries are meant to be active translations, clear fuzzy before shipping; otherwise Sphinx will ignore them and use source text (which may be desired).
doc/general/release_notes.rst (2)
16-16: Fix GitHub capitalizationUse the brand’s capitalization.
-<https://github.com/pgRouting/pgorpy/commits>`_ on Github. +<https://github.com/pgRouting/pgorpy/commits>`_ on GitHub. @@ -on Github. +on GitHub.Also applies to: 44-44
12-16: Grammar polish (optional)Add a comma after “changes” and keep tone consistent.
-To see the full list of changes check the list of `Git commits +To see the full list of changes, check the list of `Git commitspgtap/version/full_version/types_check.pg (1)
11-11: Remove redundancy: consolidate existence checks and adjust plan.
has_function('por_full_version')is redundant givenhas_function('por_full_version', ARRAY[]::text[]). Prefer the specific signature check and reduce plan to 4.Proposed diff:
-SELECT plan(5); +SELECT plan(4); -SELECT has_function('por_full_version'); SELECT has_function('por_full_version', ARRAY[]::text[]);Also applies to: 14-14
locale/pot/por_version.pot (1)
43-45: Avoid translating formatting artifacts.The "msgid "\ \"" entry appears to be a manual line-break artifact. Consider removing it from translatable text in the RST (e.g., using non-translatable roles or restructuring) to reduce noise for translators.
sql/version/version.sql (1)
25-31: Declare parallel safety.Given this is a literal-returning SQL function, it is safe to mark as PARALLEL SAFE.
Proposed diff:
LANGUAGE sql IMMUTABLE; +-- Safe for parallel execution +ALTER FUNCTION por_version() PARALLEL SAFE;Alternatively, inline:
-LANGUAGE sql IMMUTABLE; +LANGUAGE sql IMMUTABLE PARALLEL SAFE;locale/pot/sampledata.pot (2)
9-9: Match Project-Id-Version to release (0.0.0).For consistency with the release and other docs, prefer "pgORpy 0.0.0".
25-26: Capitalize Python.Use "Python" instead of "python" in the source RST so translators get the correct casing.
If you want, I can propose a patch to the source RST instead of editing POT directly.
sql/pg_controls/CMakeLists.txt (2)
4-5: Prefer list(APPEND) for clarity and to avoid accidental list re-seeding.Appending to
PROJECT_SQL_FILESis clearer and safer done in two steps: append locally, then propagate.Proposed diff:
-configure_file(${PROJECT_HEADER} ${PROJECT_HEADER}) -set(PROJECT_SQL_FILES ${PROJECT_SQL_FILES} ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_HEADER} PARENT_SCOPE) +configure_file(${PROJECT_HEADER} ${PROJECT_HEADER}) +list(APPEND PROJECT_SQL_FILES "${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_HEADER}") +set(PROJECT_SQL_FILES "${PROJECT_SQL_FILES}" PARENT_SCOPE)
8-9: Be explicit about variable substitution mode if control/header use @var@ style.If the control/header files use @var@ tokens, add
@ONLYto avoid unintended${...}substitutions; if they use${VAR}(as elsewhere), current usage is fine. Align with project-wide convention.Example (if using @var@):
-configure_file(${PROJECT_NAME_LOWER}.control ${PROJECT_NAME_LOWER}.control) +configure_file(${PROJECT_NAME_LOWER}.control ${PROJECT_NAME_LOWER}.control @ONLY)Similarly for the header, if applicable.
doc/_templates/CMakeLists.txt (1)
2-12: Minor consistency/style nits in CMake file.
- Mixed command casing: you use SET(...) and later set(...). Prefer one style for consistency.
- Consider initializing LOCAL_DOC_FILES explicitly for clarity, even though list(APPEND) works on undefined vars.
No functional issues; the logic looks good.
-SET(LOCAL_FILES +set(LOCAL_FILES layout.html ) -foreach (f ${LOCAL_FILES}) +set(LOCAL_DOC_FILES) +foreach (f ${LOCAL_FILES}) configure_file(${f} ${f} @ONLY) list(APPEND LOCAL_DOC_FILES ${CMAKE_CURRENT_BINARY_DIR}/${f}) endforeach() -set(PROJECT_DOC_FILES ${PROJECT_DOC_FILES} ${LOCAL_DOC_FILES} PARENT_SCOPE) +set(PROJECT_DOC_FILES ${PROJECT_DOC_FILES} ${LOCAL_DOC_FILES} PARENT_SCOPE)doc/general/sampledata.rst (2)
19-21: Promote the “Function to activate python…” line to a proper subsection header.As written, it reads like a stray sentence. If it’s a section header, add an underline and a blank line for valid reST.
-Function to activate python on a local environment - -.. literalinclude:: ../../tools/testers/sampledata.sql +Function to activate python on a local environment +----------------------------------------------- + +.. literalinclude:: ../../tools/testers/sampledata.sql
21-23: Add explicit syntax highlighting to literalinclude.Helps readability in the built docs.
.. literalinclude:: ../../tools/testers/sampledata.sql :start-after: -- activate_python_start :end-before: -- activate_python_end + :language: sqllocale/en/LC_MESSAGES/por_full_version.po (1)
2-2: Header version inconsistency (0.0 vs 0.0.0).Project-Id-Version says “pgORpy 0.0”, while the project version is 0.0.0. Align to avoid confusion in tooling.
-"Project-Id-Version: pgORpy 0.0\n" +"Project-Id-Version: pgORpy 0.0.0\n".github/scripts/test_shell.sh (1)
6-9: Minor: fail fast if repo root cannot be determined.Use exit 1 for consistency and explicitly set pipefail to avoid masking errors.
-DIR=$(git rev-parse --show-toplevel) +set -o pipefail +DIR=$(git rev-parse --show-toplevel) || exit 1.gitignore (1)
1-9: Consider restoring common ignores to avoid accidental commits.Given the repo includes Python tooling and docs, ignoring typical artifacts reduces noise.
# default build directory build fix_typos v[0-9]* .DS_Store .vagrant .directory *.swp +__pycache__/ +*.py[cod] +.Python +*.egg-info/ +dist/ +*.egg +.venv/ +venv/ +.idea/ +.vscode/ +doc/_build/ +build*/ +CMakeFiles/ +CMakeCache.txt +compile_commands.jsontools/testers/sampledata.sql (2)
1-3: Typo in copyright notice.Use “Copyright” and ensure consistent branding.
--- CopyRight(c) pgORpy developers +-- Copyright (c) pgORpy Contributors -- Creative Commons Attribution-Share Alike 3.0 -- License : https://creativecommons.org/licenses/by-sa/3.0/
6-8: Clarify security/privilege expectations in a comment.plpython3u requires superuser; noting that helps users understand the requirement in local setups.
CREATE OR REPLACE PROCEDURE activate_python_venv(venv text) LANGUAGE plpython3u AS $BODY$ + # Note: plpython3u is untrusted and requires superuser privileges. + # Intended for local testing environments only.locale/pot/support.pot (4)
49-51: Fix typo: “Discourse and GIS Stack Exchange”“Discurse and GIS StackExchange” has two issues: spelling (“Discourse”), and spacing (“Stack Exchange”).
Apply in source (.rst) and regenerate POT; don’t hand-edit .pot.
52-54: Clarify/condense sentence and org reference“There are two discourse categories for pgRouting products hosted on OSGeo discourse. which are of pgRouting:” is awkward and split by a period. Suggest: “There are two pgRouting discourse categories hosted on OSGeo Discourse:”.
Update in .rst then regenerate POT.
55-57: Capitalize project name consistently: pgORpyUse “pgORpy” (not “pgorpy”) across all strings.
Update in .rst and regenerate POT.
Also applies to: 61-63
76-87: Company name capitalizationConsider “Erosion Developers” (proper noun) for consistency with branding.
If the official spelling is “Erosion developers”, ignore this; otherwise update in .rst and regenerate POT.
Also applies to: 94-102
doc/general/support.rst (2)
37-39: Grammar tweak in troubleshooting wrapper“on the problem to pin point” → “around the problem to pinpoint”.
-5. It is recommended to use the following wrapper on the problem to pin point - the step that is causing the problem. +5. It is recommended to use the following wrapper around the problem to + pinpoint the step that is causing the problem.Also applies to: 40-45
54-57: Capitalize “pgORpy” and adjust bullet indentationUse “pgORpy”; also make “Subscribe” a separate bullet for consistency.
-For general questions and topics about how to use pgorpy: +For general questions and topics about how to use pgORpy: -* pgRouting-users discourse category: https://discourse.osgeo.org/c/pgrouting/pgrouting-users +* pgRouting-users discourse category: https://discourse.osgeo.org/c/pgrouting/pgrouting-users -For questions about development of pgorpy: +For questions about development of pgORpy: -* pgRouting-dev discourse category: https://discourse.osgeo.org/c/pgrouting/pgrouting-dev - - * Subscribe: https://discourse.osgeo.org/g/pgrouting-dev +* pgRouting-dev discourse category: https://discourse.osgeo.org/c/pgrouting/pgrouting-dev +* Subscribe: https://discourse.osgeo.org/g/pgrouting-devAlso applies to: 58-63
locale/pot/introduction.pot (4)
22-24: Branding: OR-Tools name/link formattingUse “OR-Tools” (capitalization and hyphen), and avoid stray quotes.
Update source .rst then regenerate POT.
Example: “connects OR-Tools https://developers.google.com/optimization to the database.”
37-39: Wording: subproject sentence“sub product of … organization organization” → “a subproject under the pgRouting organization.”
Fix in source .rst and regenerate POT.
64-66: Subject–verb agreement“The pgORpy Manual and examples is licensed …” → “… are licensed …”
Fix in .rst and regenerate POT.
100-121: Capitalize “GitHub” and company names consistentlyUse “GitHub”. Verify capitalization of “Erosion developers” vs “Erosion Developers”.
Update in .rst and regenerate POT.
Also applies to: 127-134
locale/en/LC_MESSAGES/introduction.po (1)
25-33: Mirror the same wording fixes as in the POT
- OR-Tools capitalization/link formatting.
- “subproject under the pgRouting organization.”
- “Manual and examples are …”
- “GitHub” capitalization.
Apply in source (.rst) so PO stays in sync after regeneration.
Also applies to: 43-47, 77-87, 100-107, 149-162
doc/version/por_full_version.rst (2)
29-29: Capitalize brand name consistently (“OR-Tools”).Replace “or-tools” with “OR-Tools” in prose for branding consistency.
-Returns pgORpy version information and or-tools version used. +Returns pgORpy version information and OR-Tools version used.
10-12: Index the function name for better discoverability.Use "por_full_version" in the index entry to align with the exact function name.
-.. index:: - single: full_version +.. index:: + single: por_full_versiontools/testers/setup_db.sh (1)
10-12: Harden shell options for safer failures.Use a stricter set: exit on error, unset vars, and failed pipes.
-set -e +set -euo pipefaillocale/en/LC_MESSAGES/support.po (2)
73-79: Fix typos/branding in source docs and regenerate locales.These msgids indicate issues that should be corrected in the RST source (then update .po via your locale workflow):
- “Discurse” → “Discourse”
- “pgorpy” → “pgORpy” (branding)
- Grammar: “There are two discourse categories … which are of pgRouting:” → “There are two Discourse categories for pgRouting products on OSGeo Discourse:”
- Capitalization: “pgRouting-users discourse category:” → “pgRouting-users Discourse category:”
After fixing the RST, run the locale update script to refresh this catalog.
I can propose concrete patches to doc/general/support.rst if you share it here.
Also applies to: 80-87, 88-96
117-125: Standardize company names capitalization.Ensure proper nouns are capitalized:
- “Erosion developers” → “Erosion Developers”
- Confirm the official company names/URLs are correct.
As above, prefer fixing the source RST and regenerating locales to avoid msgid drift.
If you prefer a quick fix, update msgstr here to the corrected forms while keeping msgid intact.
Also applies to: 126-134
doc/_static/custom.css (2)
7-11: Reconsider strong blue background for content wrapper (accessibility).A solid blue background across content can reduce readability and fail contrast checks depending on text color. Prefer theme variables or a subtle neutral background.
-div.content-wrapper { - background-color: blue; +div.content-wrapper { + /* Use a subtle neutral or theme variable for better contrast */ + background-color: #f8f9fa; /* or var(--bs-body-bg) if using Bootstrap variables */If you have a design system, align with it; otherwise, run a quick contrast check for typical text colors against “blue”.
52-56: Consistency: terminate last declaration with a semicolon.Optional in CSS but preferred for diffs/minifiers.
code.literal { color: #404040; background-color: #e8e8e8; - font-size: 100% + font-size: 100%; }locale/en/LC_MESSAGES/installation.po (3)
29-34: Typo and phrasing: “Operative systems” → “Operating systems”.Also consider splitting long sentence for clarity.
Proposed source RST adjustment:
- “Installation wiki …” after a period; “Operating systems.” instead of “Operative systems”.
Share the RST and I’ll provide a concrete patch.
39-41: Branding consistency: use “pgORpy”.Standardize capitalization of the project name.
- “There is no package for pgorpy.” → “There is no package for pgORpy.”
- “For general questions… how to use pgorpy” (if present in source) → “… pgORpy”
- “path/to/pgorpy/” → “path/to/pgORpy/”
Also applies to: 46-49, 149-150
100-107: Phrasing and capitalization fixes in dependencies and testing sections.
- “Postgresql” → “PostgreSQL”
- Remove double space: “CMake >= 3.17” → “CMake >= 3.17”
- “optional dependencies” → “Optional dependencies”
- “For user's documentation:” → “For user documentation:”
- “Tests are done on CI, and these are used on CI:” → “Tests run in CI; the following tools are used:”
- “Currently there is no :code:
make testand testing is done as follows” → “Currently, there is no :code:make test; testing is done as follows:”Please apply in the RST and regenerate locales to keep msgids synced.
I can batch-fix wording across the installation guide if provided.
Also applies to: 109-113, 115-121, 127-135, 145-149
doc/general/CMakeLists.txt (1)
17-19: Make the release-notes substitution idempotent.Running CMake repeatedly rewrites the already-processed
release_notes.rst, which gradually breaks formatting (second run no longer matches the:issue:pattern and leaves the back-ticks in place).
Consider guarding the substitution with a regex that first checks the file still contains:issue:markers, or generate the edited copy into${CMAKE_CURRENT_BINARY_DIR}to leave source artefacts untouched.locale/en/LC_MESSAGES/index.po (1)
24-46: Keep “fuzzy” headers up-to-date.Several entries remain fuzzy and empty, which prevents msgfmt from treating them as final. Before tagging the first release, run
msginit/msgmergeand review translations to drop the fuzzy flag.tools/scripts/get_signatures.sh (1)
12-18: Fix help text & typo.The usage block references
get_signatures.sqland spells “Execute” incorrectly.-echo "Usage: getSignatures.sh [DB_ARGS]" -echo "example" -echo "get_signatures.sql -U postgres -h localhost -p 5432 " -echo "Exeute from the root of the repository" +echo "Usage: get_signatures.sh [DB_ARGS]" +echo "Example:" +echo "./tools/scripts/get_signatures.sh -U postgres -h localhost -p 5432" +echo "Execute from the root of the repository"cmake/Version.cmake (1)
9-13: Guard against undefinedPROJECT_VERSION_DEV.On release builds that omit the var entirely, the
if (PROJECT_VERSION_DEV)test is false, but${PROJECT_VERSION_DEV}in theset(...)above expands to an empty string without error.
For clarity:if(DEFINED PROJECT_VERSION_DEV AND PROJECT_VERSION_DEV) … endif()locale/pot/installation.pot (1)
25-26: Minor wording – “Operating systems”.“Operative systems” → “Operating systems” for correct English before translators pick it up.
README.md (9)
1-1: Title wording/casing polish.Use proper OR-Tools casing and clearer phrasing.
-# pgORpy - making accessible or-tools on the database +# pgORpy - making OR-Tools accessible in the database
14-16: Avoid bare URL; make the sentence/link concise.-For the complete list of releases go to: -https://github.com/pgRouting/pgorpy/releases +For the complete list of releases, see the +[Releases page](https://github.com/pgRouting/pgorpy/releases).
19-22: Replace bare URLs with Markdown links.-* https://pgrouting.org/ -* https://pgorpy.pgrouting.org/ -* https://github.com/pgRouting/pgorpy +* [pgRouting](https://pgrouting.org/) +* [pgORpy Documentation](https://pgorpy.pgrouting.org/) +* [pgORpy on GitHub](https://github.com/pgRouting/pgorpy)
23-26: Duplicate “STATUS” heading (MD024).Rename the second heading to avoid duplication.
-## STATUS +## Project status
30-31: Typo and wording fix.“functionaily” → “functionality”; standardize OR-Tools name.
-pgORpy extends the pgRouting/PostGIS/PostgreSQL geospatial database to provide OR-tools functionaily on the PostgreSQL -database. +pgORpy extends the pgRouting/PostGIS/PostgreSQL geospatial database to provide +OR-Tools functionality in PostgreSQL.
43-49: Heading style consistency (MD003) and capitalization.Use atx-style heading and fix PostgreSQL capitalization. Consider clarifying Sphinx version operator.
-Building requirements --------------------- +### Building requirements * Perl -* Postgresql not on EOL +* PostgreSQL (supported, not EOL) * CMake >= 3.17 -* Sphinx > 4.0.0 +* Sphinx >= 4.0.0 * OR-tools == 9.10.4067
51-59: Improve build instructions and remove hard tabs (MD010).Recommend explicit build+install with CMake and fenced code block.
-For Linux - - mkdir build - cd build - cmake .. - sudo make install +### Building on Linux + +```sh +mkdir -p build +cd build +cmake .. +cmake --build . --parallel +sudo cmake --install . +```
62-66: Use fenced code block and spaces (no hard tabs).-Create the extension - - createdb mydatabase - psql mydatabase -c "CREATE EXTENSION pgorpy CASCADE" +Create the extension + +```sh +createdb mydatabase +psql mydatabase -c "CREATE EXTENSION pgorpy CASCADE" +```
69-69: Replace bare URL with Markdown link (MD034).-See online documentation: https://pgorpy.pgrouting.org/latest/en/index.html +See online documentation: <https://pgorpy.pgrouting.org/latest/en/index.html>doc/general/introduction.rst (7)
16-18: RST link markup and naming for OR-Tools.Use proper inline link markup and casing.
-pgORpy is a `PostgreSQL <https://www.postgresql.org>`__ extension that connects -'OR-tools <https://developers.google.com/optimization>' to the database. +pgORpy is a `PostgreSQL <https://www.postgresql.org>`__ extension that integrates +`OR-Tools <https://developers.google.com/optimization>`__ with the database.
26-28: Wording: “sub product … organization organization”.Simplify to “subproject of the pgRouting organization.”
-pgORpy is a sub product of `pgRouting organization <https://pgrouting.org>`__ -organization. +pgORpy is a subproject of the `pgRouting organization <https://pgrouting.org>`__.
47-49: Subject-verb agreement and link style consistency.“The … Manual and examples is” → “are”; unify to use double-underscore links like above.
- - The pgORpy Manual and examples is licensed under a `Creative Commons - Attribution-Share Alike 3.0 License - <https://creativecommons.org/licenses/by-sa/3.0/>`_. + - The pgORpy Manual and examples are licensed under a `Creative Commons + Attribution-Share Alike 3.0 License + <https://creativecommons.org/licenses/by-sa/3.0/>`__.
52-52: Minor grammar.Add a comma after “In general”.
-In general license information is included in the header of each source file. +In general, license information is included in the header of each source file.
66-66: “Github” → “GitHub”.- - Github actions for documentation publishing and release + - GitHub actions for documentation publishing and releaseAlso applies to: 93-93
105-107: Make sponsor names consistent with earlier linked form.-- Erosion developers -- Paragon Corporation +- `Erosion developers <https://www.erosion.dev/>`__ +- `Paragon Corporation <https://www.paragoncorporation.com/>`__
112-117: Replace bare URLs with proper RST links.-* The latest software, documentation and news items are available at the pgORpy - web site https://pgorpy.pgrouting.org. -* PostgreSQL database server at the PostgreSQL main site - https://www.postgresql.org. -* Download pgORpy extension at the pgRouting organization github - https://github.com/pgRouting/pgorpy +* The latest software, documentation and news items are available at the + pgORpy web site: https://pgorpy.pgrouting.org. +* PostgreSQL main site: https://www.postgresql.org. +* Download pgORpy extension: https://github.com/pgRouting/pgorpytools/testers/pg_prove_tests.sh (3)
72-75: Comment grammar and behavior clarity.Minor grammar fix; also clarify that the DB is removed only if CLEANDB is set.
-# database wont be removed unless script does not fails +# Database will be removed only if the script succeeds and CLEANDB is set. if [ -n "$CLEANDB" ]; then dropdb --if-exists "${PGPORT[@]}" "${PGUSER[@]}" "${PGDATABASE}" fi
10-10: Optional: strengthen bash safety.Consider adding nounset and pipefail for more robust failure detection.
-set -e +set -euo pipefail
15-45: Optional: provide -h/--help usage and validate required tools exist.Improve UX and early-fail if pg_prove/psql are missing.
doc/general/index.rst (2)
37-45: Put all General pages in the same toctree
sampledatais listed outside the first.. toctree::, so it won’t appear in the navigation panel. Indent it under the sameGeneraltoctree (or create a dedicated one) to keep sidebar navigation consistent.
51-57: Replace “:doc:functions” with a real section headingUsing a
:doc:role as a pseudo-heading renders poorly and may trigger Sphinx “Title level inconsistent” warnings. Add a proper heading (e.g., “Functions”) and leave the.. include::/toctree directives underneath.locale/en/LC_MESSAGES/release_notes.po (1)
6-9: Drop the#, fuzzyflag before shippingThe
fuzzymarker tells gettext the entries are unverified; it will skip them during compilation. Remove the flag-line (or review and unfuzzy individual msgs) so English strings are always installed..github/scripts/update_locale.sh (1)
27-31: Quote path substitution for reliabilityIf a filename ever contains spaces, the pipeline may split words. Use
while IFS= read -r lineand double-quote the variable in theperlcall to stay safe.sql/scripts/build-extension-file.pl (1)
34-40: Remove unused modules
Data::DumperandFile::Findare imported but never used, increasing startup time and hiding real dependencies.tools/scripts/fix_typos.sh (2)
50-53: Initialize EXCLUDED_FILES without a leading comma.Starting with an empty variable yields a leading comma, which is brittle. Initialize in one assignment.
-EXCLUDED_FILES="$EXCLUDED_FILES,*/fix_typos/*" -EXCLUDED_FILES="$EXCLUDED_FILES,*/.git/*" -EXCLUDED_FILES="$EXCLUDED_FILES,*/build/*" -EXCLUDED_FILES="$EXCLUDED_FILES,*/locale/*" +EXCLUDED_FILES="*/fix_typos/*,*/.git/*,*/build/*,*/locale/*"
1-2: Add strict mode for safer scripting.Enable -e and -u to fail fast on errors/undefined vars.
#!/bin/sh # -*- coding: utf-8 -*- +set -eu.github/scripts/test_signatures.sh (1)
3-4: Fix typos in comments: “mayors” → “majors”.Small clarity improvement.
-# This test checks that signatures within mayors +# This test checks that signatures within majors ... -# comparing within same mayors only +# comparing within same majors onlyAlso applies to: 23-24
doc/general/installation.rst (3)
15-19: Grammar and clarity: “Operative systems” and sentence structure.Tighten wording and fix terminology.
-Instructions for downloading and installing binaries for different Operative -systems instructions and additional notes and corrections not included in this -documentation can be found in `Installation wiki +Instructions for downloading and installing binaries for different operating +systems, plus additional notes not included in this documentation, can be found in the `Installation wiki <https://github.com/pgRouting/${PROJECT_NAME_LOWER}/wiki/Notes-on-Download%2C-Installation-and-building-pgORpy>`__
99-103: Typo: “Postgresql” → “PostgreSQL”.Brand capitalization.
-* Postgresql version >= ${POSTGRESQL_MINIMUM_VERSION} +* PostgreSQL version >= ${POSTGRESQL_MINIMUM_VERSION}
168-174: Duplicate rubric title.Second “Compare the documentation results” should describe running pgTAP tests.
-.. rubric:: Compare the documentation results +.. rubric:: Run pgTAP tests.github/workflows/files.yml (2)
7-10: Fix typos in workflow comments.Minor clarity improvements.
-# - the release_notes and NEWS are consitent +# - the release_notes and NEWS are consistent ... -# Does not makes a compilation +# Does not perform a compilation
41-46: Make NEWS checks more robust (quoting and grep).Quote the version, and avoid grep -P dependency; fail cleanly if version missing.
- .github/scripts/notes2news.pl - if git status | grep 'NEWS.md'; then echo "NEWS.md is not up to date"; exit 1; fi - if ! grep -q $(grep -Po '(?<=project\(pgORpy VERSION )[^;]+' CMakeLists.txt) NEWS.md; then echo "Missing section in NEWS.md"; exit 1; fi + .github/scripts/notes2news.pl + if ! git diff --quiet -- NEWS.md; then echo "NEWS.md is not up to date"; exit 1; fi + ver="$(grep -Eo 'project\(pgORpy VERSION [^)]+' CMakeLists.txt | awk '{print $3}')" + if [ -z "$ver" ]; then echo "Could not parse version from CMakeLists.txt"; exit 1; fi + if ! grep -qF "$ver" NEWS.md; then echo "Missing section in NEWS.md"; exit 1; fi.github/workflows/macos.yml (1)
25-27: Typo and TODO clarity.Fix “disbale” and reference a tracking issue for plpython3u support.
- # disbale action, TODO install plpython3u.control + # disable action; TODO: install plpython3u.control (track in issue #<id>)I can open a follow-up issue describing the steps to enable plpython3u on macOS CI. Want me to draft it?
tools/developer/run.sh (1)
28-33: Variable shadowing ofDIRobscures repo root.
DIRis first the repo root, then reassigned to the pgTAP directory list. Reuse a different name (e.g.,TAP_DIR) to avoid accidental path misuse in later steps.CMakeLists.txt (1)
57-60: Undefined variable logged.
DOXYGEN_MINIMUM_VERSIONis printed but never set, producing an empty status message and confusing users. Either define it or drop the log line.tools/developer/taptest.sh (2)
41-45: Hard-codedPGUSERlimits portability.
PGUSER=vickyforces the local user. Respect existing environment or accept a flag instead:-PGUSER=vicky +PGUSER="${PGUSER:-$USER}"
30-36:grep … | awk '{print $3}'is brittle.If the project line format changes, version extraction breaks. Consider a safer pattern, e.g.:
VERSION=$(grep -Po '(?<=project\(pgORpy VERSION )[^)]+').github/workflows/website.yml (1)
17-18: Typo and always-false condition disable the job.
disbale→disableand theif: ${{ false }}statement prevents any run. Confirm this is intentional; otherwise drop the condition or use an environment toggle.doc/conf.py.in (1)
108-111: Verify static assets existEnsure
_static/custom.cssand_static/page_history.jsare present; otherwise Sphinx will warn..github/workflows/locale.yml (2)
96-103: Harden push step to reduce race conditionsIf multiple runs push concurrently,
git pushmay fail. Rebase before pushing.- name: Update locale run: | bash .github/scripts/update_locale.sh # Add the files, commit and push git diff --staged --quiet || git commit -m "Update locale: commit for hash ${{ env.GIT_HASH }}" - git restore . # Remove the unstaged changes before rebasing - git push + git restore . # Remove the unstaged changes before rebasing + git pull --rebase + git push
41-47: PostgreSQL version extraction: confirm regex behavior across versionsThe regex extracts the major version only (e.g., 16 from "psql (PostgreSQL) 16.3 (...)"). This looks fine; just a heads-up to keep it in sync with GitHub runner images.
sql/scripts/build-extension-update-files.pl (4)
84-93: Version validation and extraction depend on fixed regexesOnce regexes are corrected, extracting
$new_mayor/$old_mayorand$new_minor/$old_minorwill work. Keep usingversion->parsefor ordering; it’s robust.Run a quick check locally with sample inputs:
0.0.1 -> 0.0.0,1.2.0 -> 1.1.9.
219-221: Fix incorrect error message path when creating the output fileThe die message references a malformed path and duplicates
$PROJECT.- open(OUT, ">$output_file_name") - || die "ERROR: failed to create '$output_directory/$PROJECT-$PROJECT--$old_version--$new_version.sql' : $!\n"; + open(OUT, ">$output_file_name") + || die "ERROR: failed to create '$output_file_name' : $!\n";
24-40: Duplicateeval 'exec /usr/bin/perl -S $0 …'blocksThere are two identical guard lines; keep only one.
126-205: Behavior across minor vs major changes: confirm drop policyYou enforce no signature removals within same major (die) and drop only when major changes. That’s consistent; ensure this aligns with project policy and tests.
I can update
.github/scripts/test_signatures.shor add pgTAP coverage if needed.tools/testers/general_pgtap_tests.sql (1)
15-21: Assumption about por_full_version() structure
min_lib_versionselectslibraryfrompor_full_version(). Confirm that function returns a row with alibraryfield. If it returns text, this SELECT will fail.If needed, I can adjust the function or the query to parse the string format actually returned.
.github/workflows/documentation.yml (1)
61-79: Matrix gating and linkcheck control look good; minor nitGood use of gating to always build English and selectively build other languages. Consider caching pip/apt where possible to speed up runs.
.github/workflows/ubuntu.yml (1)
41-44: Unusedosmatrix dimension
matrix.osis declared butruns-onis hard-coded toubuntu-latest, so the job never varies. Either drop theosaxis or setruns-on: ${{ matrix.os }}for the intended coverage.Also applies to: 36-36
sql/CMakeLists.txt (1)
17-18: Minor typoComment should read “build scripts”, not “build scrits”.
.github/workflows/release.yml (1)
14-15: Typo: “disbale”Fix spelling to “disable” to keep comments professional.
doc/_static/page_history.js (1)
71-75: Implicit globallatestand repeatedvar i– switch tolet/const
latest = versionsArr[0];(Line 71) assigns to the global object; re-declaringvar iin nested loops also triggersnoRedeclare.- latest = versionsArr[0]; + const latest = versionsArr[0];And use
for (let i = 0; …)throughout.
Improves safety and satisfies linters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
doc/_static/favicon.icois excluded by!**/*.icodoc/_static/images/ccbysa.pngis excluded by!**/*.pngdoc/_static/images/favicon.icois excluded by!**/*.icodoc/_static/images/pgORpy.pngis excluded by!**/*.png
📒 Files selected for processing (98)
.editorconfig(1 hunks).github/scripts/notes2news.pl(1 hunks).github/scripts/remove_obsolete_entries.sh(1 hunks).github/scripts/test_license.sh(1 hunks).github/scripts/test_shell.sh(1 hunks).github/scripts/test_signatures.sh(1 hunks).github/scripts/update_locale.sh(1 hunks).github/workflows/documentation.yml(1 hunks).github/workflows/files.yml(1 hunks).github/workflows/locale.yml(1 hunks).github/workflows/macos.yml(1 hunks).github/workflows/queries.yml(1 hunks).github/workflows/release.yml(1 hunks).github/workflows/ubuntu.yml(1 hunks).github/workflows/website.yml(1 hunks).gitignore(1 hunks)CMakeLists.txt(1 hunks)CODE_OF_CONDUCT.md(1 hunks)CONTRIBUTING.md(1 hunks)NEWS.md(1 hunks)README.md(1 hunks)cmake/Configure.cmake(1 hunks)cmake/Version.cmake(1 hunks)configuration.conf(1 hunks)doc/CMakeLists.txt(1 hunks)doc/_static/CMakeLists.txt(1 hunks)doc/_static/custom.css(1 hunks)doc/_static/images/CMakeLists.txt(1 hunks)doc/_static/page_history.js(1 hunks)doc/_templates/CMakeLists.txt(1 hunks)doc/_templates/layout.html(1 hunks)doc/conf.py.in(1 hunks)doc/forward.html(1 hunks)doc/general/CMakeLists.txt(1 hunks)doc/general/functions.rst(1 hunks)doc/general/index.rst(1 hunks)doc/general/installation.rst(1 hunks)doc/general/introduction.rst(1 hunks)doc/general/release_notes.rst(1 hunks)doc/general/sampledata.rst(1 hunks)doc/general/support.rst(1 hunks)doc/requirements.txt(1 hunks)doc/version/CMakeLists.txt(1 hunks)doc/version/por_full_version.rst(1 hunks)doc/version/por_version.rst(1 hunks)docqueries/version/CMakeLists.txt(1 hunks)docqueries/version/full_version.pg(1 hunks)docqueries/version/full_version.result(1 hunks)docqueries/version/test.conf(1 hunks)docqueries/version/version.pg(1 hunks)docqueries/version/version.result(1 hunks)gitattributes(1 hunks)locale/.gitignore(1 hunks)locale/en/LC_MESSAGES/functions.po(1 hunks)locale/en/LC_MESSAGES/index.po(1 hunks)locale/en/LC_MESSAGES/installation.po(1 hunks)locale/en/LC_MESSAGES/introduction.po(1 hunks)locale/en/LC_MESSAGES/por_full_version.po(1 hunks)locale/en/LC_MESSAGES/por_version.po(1 hunks)locale/en/LC_MESSAGES/release_notes.po(1 hunks)locale/en/LC_MESSAGES/sampledata.po(1 hunks)locale/en/LC_MESSAGES/support.po(1 hunks)locale/pot/functions.pot(1 hunks)locale/pot/index.pot(1 hunks)locale/pot/installation.pot(1 hunks)locale/pot/introduction.pot(1 hunks)locale/pot/por_full_version.pot(1 hunks)locale/pot/por_version.pot(1 hunks)locale/pot/release_notes.pot(1 hunks)locale/pot/sampledata.pot(1 hunks)locale/pot/support.pot(1 hunks)or-tools-license(1 hunks)pgtap/version/full_version/types_check.pg(1 hunks)pgtap/version/version/types_check.pg(1 hunks)requirements.txt(1 hunks)sql/CMakeLists.txt(1 hunks)sql/pg_controls/CMakeLists.txt(1 hunks)sql/pg_controls/pgorpy-header.sql(1 hunks)sql/pg_controls/pgorpy.control(1 hunks)sql/scripts/CMakeLists.txt(1 hunks)sql/scripts/build-extension-file.pl(1 hunks)sql/scripts/build-extension-update-files.pl(1 hunks)sql/sigs/CMakeLists.txt(1 hunks)sql/sigs/pgorpy--0.0.sig(1 hunks)sql/version/CMakeLists.txt(1 hunks)sql/version/full_version.sql(1 hunks)sql/version/version.sql(1 hunks)tools/developer/run.sh(1 hunks)tools/developer/taptest.sh(1 hunks)tools/scripts/fix_typos.sh(1 hunks)tools/scripts/get_signatures.sh(1 hunks)tools/scripts/typos_whitelist.txt(1 hunks)tools/testers/doc_queries_generator.pl(1 hunks)tools/testers/general_pgtap_tests.sql(1 hunks)tools/testers/no_crash_test.sql(1 hunks)tools/testers/pg_prove_tests.sh(1 hunks)tools/testers/sampledata.sql(1 hunks)tools/testers/setup_db.sh(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md
[style] ~3-~3: As an alternative to the over-used intensifier ‘really’, consider replacing this phrase.
Context: # How to contribute We are really glad you are reading this because we need vo...
(EN_WEAK_ADJECTIVE)
[style] ~48-~48: The word ‘bigger’ tends to be overused. Consider an alternative.
Context: ...essages are fine for small changes, but bigger changes should have more information. Fill in ...
(BIG_DIFFERENCE)
tools/scripts/typos_whitelist.txt
[style] ~21-~21: Using many exclamation marks might seem excessive (in this case: 7 exclamation marks for a text that’s 2294 characters long)
Context: ... "PRT!NAM", sizeOfFields[nFields] += WriteSubFieldStr(fd, osBaseFileName, 8); /* NAM / sizeOfFields[nFields] += WriteSubFieldStr(fd, osNAM.c_str(), 8); / NAM */ "NAM!STR!PRT!SWO!SWA!NEO!NEA", sizeOfFields[nFields] += W...
(EN_EXCESSIVE_EXCLAMATION)
README.md
[grammar] ~30-~30: Ensure spelling is correct
Context: ...geospatial database to provide OR-tools functionaily on the PostgreSQL database. This libra...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
CODE_OF_CONDUCT.md
[style] ~32-~32: Consider a different adverb to strengthen your wording.
Context: ...e feels good about joining, and we will always work to treat everyone well. No matter ...
(ALWAYS_CONSTANTLY)
[style] ~62-~62: ‘for the benefit’ might be wordy. Consider a shorter alternative.
Context: ... work of others. When we make something for the benefit of OSGeo, we are willing to explain t...
(EN_WORDINESS_PREMIUM_FOR_THE_BENEFIT)
[style] ~84-~84: Consider a different adverb to strengthen your wording.
Context: ...iently as possible. Short emails should always strive to be empathetic, welcoming, f...
(ALWAYS_CONSTANTLY)
[style] ~129-~129: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ... degrade the quality of the discussion. Should there be difficulties in dealing with t...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.17.2)
NEWS.md
10-10: Spaces inside link text
(MD039, no-space-in-links)
14-14: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
README.md
15-15: Bare URL used
(MD034, no-bare-urls)
19-19: Bare URL used
(MD034, no-bare-urls)
20-20: Bare URL used
(MD034, no-bare-urls)
21-21: Bare URL used
(MD034, no-bare-urls)
23-23: Multiple headings with the same content
(MD024, no-duplicate-heading)
43-43: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
55-55: Hard tabs
Column: 1
(MD010, no-hard-tabs)
56-56: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
64-64: Hard tabs
Column: 1
(MD010, no-hard-tabs)
65-65: Hard tabs
Column: 1
(MD010, no-hard-tabs)
69-69: Bare URL used
(MD034, no-bare-urls)
🪛 actionlint (1.7.7)
.github/workflows/queries.yml
29-29: "tags" section should not be empty
(syntax-check)
88-88: property "psql" is not defined in object type {release: string}
(expression)
.github/workflows/release.yml
128-128: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/documentation.yml
25-25: "tags" section should not be empty
(syntax-check)
🪛 Biome (2.1.2)
doc/_static/page_history.js
[error] 91-91: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (23)
tools/scripts/typos_whitelist.txt (1)
24-24: Static analysis: exclamation marks warning is a false positive for whitelist contentThis file intentionally whitelists strings verbatim; the “excessive exclamation marks” style warning can be ignored here.
requirements.txt (1)
1-1: All ortools pins are consistentOnly one occurrence of an ortools version pin was found—in requirements.txt—so every workflow installing from that file will use 9.10.4067. No further changes needed here.
You can still consider adding a hash to the requirement for stronger supply-chain guarantees when you’re ready.locale/.gitignore (1)
1-2: LGTMIgnoring compiled .mo files and Sphinx .doctrees here is appropriate.
sql/sigs/pgorpy--0.0.sig (1)
1-2: LGTM: function signatures introducedSignatures for por_full_version() and por_version() are present and consistent with the PR scope.
sql/sigs/CMakeLists.txt (1)
1-4: No propagation needed for SIGNATURE_FILES
Our search shows that SIGNATURE_FILES is only defined in sql/sigs/CMakeLists.txt and isn’t consumed in sql/CMakeLists.txt or anywhere else in the repo. Since the parent CMakeLists doesn’t reference this variable, adding a PARENT_SCOPE set is unnecessary.Likely an incorrect or invalid review comment.
docqueries/version/version.result (1)
1-15: LGTM: result transcript for por_version() is consistent with 0.0.0Output structure (BEGIN/SET/SELECT/ROLLBACK) and the returned version string look correct for docs/tests.
docqueries/version/version.pg (1)
1-7: LGTM: minimal query for por_version()The query file follows the expected pattern and is sufficient for the docqueries pipeline.
docqueries/version/test.conf (1)
3-12: All referenced test files are present
Bothdocqueries/version/full_version.pgand its corresponding result file exist in the repository, so the manifest reference is valid. No action required.doc/_static/CMakeLists.txt (1)
13-14: Order is fine; just confirming aggregation behavior.add_subdirectory(images) then appending LOCAL_DOC_FILES to PROJECT_DOC_FILES in parent scope is correct and won’t drop subdir contributions.
sql/pg_controls/pgorpy-header.sql (1)
3-5: Confirm YEAR substitution.Ensure CMake (or the templating step) sets ${YEAR}. If unset, the final artifact will leak the literal token.
pgtap/version/version/types_check.pg (1)
11-17: LGTM: function existence and return type checks.The pgTAP assertions for existence and return type are sound and match the expected zero-arg text-returning function.
locale/pot/functions.pot (1)
25-30: Cross-check doc targets exist.Ensure
:doc:\por_version`and:doc:`por_full_version`` point to the actual RST file names/labels to avoid Sphinx broken links.If helpful, I can add a docs link-check step suggestion or search for the targets in the repo.
sql/version/full_version.sql (1)
32-36: Doc link placeholder checkEnsure ${PROJECT_DOC_LINK} expands correctly in release builds and points to por_full_version.html.
pgtap/version/full_version/types_check.pg (1)
18-25: No changes needed for OUT parameters
Thepor_full_version()function is already defined with named OUT parameters(version TEXT, or_tools TEXT), so bothfunction_args_eqandfunction_types_eqwill correctly detect the parameter names and types. No alignment between the tests and the function signature is required.sql/version/version.sql (2)
24-36: LGTM: minimal, immutable version function fits extension best practices.The function is simple, IMMUABLE, and uses configured literals—good for stability and upgrade paths. Comment with doc link is a nice touch.
32-36: por_version() confirmed in signature; no upgrade scripts require updates.
sql/sigs/pgorpy--0.0.sigcontains apor_version()entry.- No
pgorpy--*--*.sqlupgrade scripts exist undersql/.locale/en/LC_MESSAGES/por_full_version.po (1)
6-20: Empty English translations won’t blank out content
Our test with Python’s gettext (which Sphinx uses) shows that entries with an emptymsgstrare treated as untranslated and fall back to the originalmsgid. Having an English.powith empty translations therefore will not replace your source strings with blanks. No changes are needed here.Likely an incorrect or invalid review comment.
.gitignore (1)
4-4: Verify the v[0-9] pattern won’t hide needed versioned dirs/files.*If you plan to keep versioned directories/files in-tree (e.g., docs or release notes), this pattern may ignore them.
Would you like a quick repo scan to list paths currently matching v[0-9]* so we can whitelist as needed?
tools/testers/setup_db.sh (1)
22-25: LGTM on SQL bootstrap sequence paths.Using DIR to anchor file paths and ON_ERROR_STOP are good practices. No issues.
tools/scripts/get_signatures.sh (1)
21-22:grep -Plimits portability.The Perl-regex flag isn’t available on default macOS/BSD grep. Consider
grep -Eplussed, or usecmake -Pto query the version instead.sql/scripts/build-extension-file.pl (1)
86-106: Generic comment-stripping regex can eat SQL codeThe
/\* … \*/pattern will remove all block comments, including inline license headers already cleaned, and could erase legit commented-out SQL or documentation. Consider restricting the third regex to headers starting at file top or prefixed with a marker to avoid unintentional code loss.CODE_OF_CONDUCT.md (1)
1-3: Resolved—no technical issues foundThe document is fine as-is; stylistic suggestions from LanguageTool are optional and do not block merging.
tools/testers/doc_queries_generator.pl (1)
367-386: Logic error inversion_greater_eq– scalars clobber arraysInside the
whileloop the same names$a/$bare reused as scalars, destroying the array references after the first iteration and yielding incorrect comparisons.- $a = shift @a || 0; - $b = shift @b || 0; + my $ai = shift @a // 0; + my $bi = shift @b // 0; + $va = $va * 1000 + $ai; + $vb = $vb * 1000 + $bi;Likely an incorrect or invalid review comment.
| #!/bin/bash | ||
| # ------------------------------------------------------------------------------ | ||
| # pgORpy Scripts | ||
| # Copyright(c) pgORpy Contributors | ||
| # | ||
| # Remove all the obsolete entries, i.e. lines starting with #~ from .po files | ||
| # ------------------------------------------------------------------------------ | ||
|
|
||
| # For all the chapter files | ||
| for file in locale/en/LC_MESSAGES/*.po; do | ||
| if grep -q '#~' "$file"; then | ||
| perl -pi -0777 -e 's/#~.*//s' "$file" | ||
| git add "$file" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Harden script: strict mode, nullglob, broader locale coverage, anchored grep
Improve robustness when no files match and support all locales.
#!/bin/bash
-# ------------------------------------------------------------------------------
+# ------------------------------------------------------------------------------
# pgORpy Scripts
# Copyright(c) pgORpy Contributors
#
# Remove all the obsolete entries, i.e. lines starting with #~ from .po files
# ------------------------------------------------------------------------------
-# For all the chapter files
-for file in locale/en/LC_MESSAGES/*.po; do
- if grep -q '#~' "$file"; then
- perl -pi -0777 -e 's/#~.*//s' "$file"
- git add "$file"
- fi
-done
+set -euo pipefail
+shopt -s nullglob
+
+# Process all locales
+for file in locale/*/LC_MESSAGES/*.po; do
+ if grep -q '^#~' "$file"; then
+ # Delete only lines that start with '#~' (obsolete entries)
+ sed -i '/^#~/d' "$file"
+ git add "$file"
+ fi
+done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/bin/bash | |
| # ------------------------------------------------------------------------------ | |
| # pgORpy Scripts | |
| # Copyright(c) pgORpy Contributors | |
| # | |
| # Remove all the obsolete entries, i.e. lines starting with #~ from .po files | |
| # ------------------------------------------------------------------------------ | |
| # For all the chapter files | |
| for file in locale/en/LC_MESSAGES/*.po; do | |
| if grep -q '#~' "$file"; then | |
| perl -pi -0777 -e 's/#~.*//s' "$file" | |
| git add "$file" | |
| fi | |
| done | |
| #!/bin/bash | |
| # ------------------------------------------------------------------------------ | |
| # pgORpy Scripts | |
| # Copyright(c) pgORpy Contributors | |
| # | |
| # Remove all the obsolete entries, i.e. lines starting with #~ from .po files | |
| # ------------------------------------------------------------------------------ | |
| set -euo pipefail | |
| shopt -s nullglob | |
| # Process all locales | |
| for file in locale/*/LC_MESSAGES/*.po; do | |
| if grep -q '^#~' "$file"; then | |
| # Delete only lines that start with '#~' (obsolete entries) | |
| sed -i '/^#~/d' "$file" | |
| git add "$file" | |
| fi | |
| done |
🤖 Prompt for AI Agents
In .github/scripts/remove_obsolete_entries.sh lines 1 to 15, enhance the script
by enabling strict mode (set -euo pipefail) to catch errors early, use 'shopt -s
nullglob' to handle cases where no .po files exist without errors, and expand
the file loop to cover all locale directories (e.g., locale/*/LC_MESSAGES/*.po).
Also, modify the grep command to anchor the pattern to line starts for precise
matching of lines beginning with '#~'.
| # For all the chapter files | ||
| for file in locale/en/LC_MESSAGES/*.po; do | ||
| if grep -q '#~' "$file"; then | ||
| perl -pi -0777 -e 's/#~.*//s' "$file" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: regex wipes content from first '#~' to EOF
The Perl command uses a greedy dot-star with the singleline modifier, which will delete everything from the first “#” to the end of the file. Remove only lines that start with “#”.
- perl -pi -0777 -e 's/#~.*//s' "$file"
+ # Delete only lines that start with '#~' (obsolete entries)
+ sed -i '/^#~/d' "$file"🤖 Prompt for AI Agents
In .github/scripts/remove_obsolete_entries.sh at line 12, the Perl regex uses a
greedy pattern that removes all content from the first "#~" to the end of the
file. Modify the regex to only delete lines that start with "#~" by matching and
removing entire lines beginning with "#~" instead of using a greedy dot-star
pattern.
| EXCLUDE_LIST="txt|sig|png|jpeg|_static|md|control|html|cfg" | ||
|
|
||
| mylicensecheck() { | ||
| licensecheck -r --copyright -l 30 --tail 0 -i "$EXCLUDE_LIST" "$1" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Race conditions and fragile pipeline; tighten robustness
Parallel invocations aren’t waited; output can be truncated. Also add strict modes and robust doc scan.
#!/usr/bin/env bash
# This test checks that all source files correctly have license headers
-EXCLUDE_LIST="txt|sig|png|jpeg|_static|md|control|html|cfg"
+set -euo pipefail
+
+EXCLUDE_LIST="txt|sig|png|jpg|jpeg|svg|gif|_static|md|control|html|cfg|pdf"
mylicensecheck() {
- licensecheck -r --copyright -l 30 --tail 0 -i "$EXCLUDE_LIST" "$1"
+ licensecheck -r --copyright -l 30 --tail 0 -i "$EXCLUDE_LIST" "$1"
}
DIR=$(git rev-parse --show-toplevel)
-pushd "${DIR}" > /dev/null || exit
-missing=$(! { mylicensecheck src & mylicensecheck sql & mylicensecheck include;} | grep "No copyright\|UNKNOWN")
-missing1=$(mylicensecheck doc | grep "No copyright")
-missing2=$(grep --files-without-match 'Creative Commons' doc/*/*.rst)
-popd > /dev/null || exit
+pushd "${DIR}" > /dev/null || exit
+# Run sequentially to avoid race conditions; parallelize later with proper waits if needed.
+missing=$({ mylicensecheck src; mylicensecheck sql; mylicensecheck include; } \
+ | grep -E "No copyright|UNKNOWN" || true)
+missing1=$(mylicensecheck doc | grep -F "No copyright" || true)
+# Find all .rst files and list those missing the CC notice
+missing2=$(find doc -type f -name '*.rst' -print0 | xargs -0 grep -L 'Creative Commons' || true)
+popd > /dev/null || exit
#mylicensecheck doc
error=0
-if [[ $missing ]]; then
+if [[ -n "${missing}" ]]; then
echo " ****************************************************"
echo " *** Found source files without valid license headers"
echo " ****************************************************"
- echo "$missing"
+ echo "${missing}"
error=1
fi
-if [[ $missing1 ]]; then
+if [[ -n "${missing1}" ]]; then
echo " ****************************************************"
echo " *** Found documentation files without copyright"
echo " ****************************************************"
- echo "$missing1"
+ echo "${missing1}"
error=1
fi
-if [[ $missing2 ]]; then
+if [[ -n "${missing2}" ]]; then
echo " ****************************************************"
echo " *** Found documentation files without valid license headers"
echo " ****************************************************"
- echo "$missing2"
+ echo "${missing2}"
error=1
fi
exit $errorNotes:
- set -euo pipefail hardens the script.
- Sequential checks avoid incomplete output; if you prefer parallel, collect to temp files and wait.
- grep -E with || true prevents non-zero exit from breaking the script.
Also applies to: 11-17, 19-45
🤖 Prompt for AI Agents
In .github/scripts/test_license.sh around lines 5 to 9, the script lacks strict
mode settings and runs license checks in parallel without waiting, causing
potential output truncation and race conditions. Add 'set -euo pipefail' at the
top of the script to enable strict error handling. Modify the license check
function to run sequentially or, if parallel execution is needed, redirect
outputs to temporary files and wait for all to finish before processing. Also,
update grep commands to use 'grep -E ... || true' to prevent non-zero exit codes
from breaking the script.
| for f in $(git ls-files | grep '\.sh') | ||
| do | ||
| result=$(shellcheck "${f}") | ||
|
|
||
| if [[ $result ]]; then | ||
| echo "$result" | ||
| echo " *** shellcheck found script errors while processing $f" | ||
| code=1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Handle filenames safely and rely on exit codes from shellcheck.
Current loop splits on whitespace and infers errors from output presence. Use -z with git ls-files and check shellcheck’s exit code.
-for f in $(git ls-files | grep '\.sh')
-do
- result=$(shellcheck "${f}")
-
- if [[ $result ]]; then
- echo "$result"
- echo " *** shellcheck found script errors while processing $f"
- code=1
- fi
-done
+while IFS= read -r -d '' f; do
+ if ! shellcheck "$f"; then
+ echo " *** shellcheck found script errors while processing $f"
+ code=1
+ fi
+done < <(git ls-files -z -- '*.sh')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for f in $(git ls-files | grep '\.sh') | |
| do | |
| result=$(shellcheck "${f}") | |
| if [[ $result ]]; then | |
| echo "$result" | |
| echo " *** shellcheck found script errors while processing $f" | |
| code=1 | |
| fi | |
| done | |
| while IFS= read -r -d '' f; do | |
| if ! shellcheck "$f"; then | |
| echo " *** shellcheck found script errors while processing $f" | |
| code=1 | |
| fi | |
| done < <(git ls-files -z -- '*.sh') |
🤖 Prompt for AI Agents
In .github/scripts/test_shell.sh around lines 11 to 20, the current loop uses
command substitution which splits filenames on whitespace and checks for errors
by inspecting output, which is unsafe and unreliable. Modify the loop to read
filenames safely by using a while read loop with null delimiters from git
ls-files -z, and check shellcheck's exit code directly instead of output
presence. This ensures filenames with spaces are handled correctly and error
detection is accurate.
| configure_file("configuration.conf" "configuration.conf") | ||
| file(STRINGS configuration.conf PROJECT_CONFIGURATION_FILE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Locate configuration.conf robustly (use CMAKE_CURRENT_LIST_DIR) and avoid implicit paths.
When this script is included from another directory, relative paths can break. Use explicit source/binary dirs and COPYONLY.
-configure_file("configuration.conf" "configuration.conf")
-file(STRINGS configuration.conf PROJECT_CONFIGURATION_FILE)
+configure_file(
+ "${CMAKE_CURRENT_LIST_DIR}/configuration.conf"
+ "${CMAKE_CURRENT_BINARY_DIR}/configuration.conf"
+ COPYONLY
+)
+file(STRINGS "${CMAKE_CURRENT_BINARY_DIR}/configuration.conf" PROJECT_CONFIGURATION_FILE)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| configure_file("configuration.conf" "configuration.conf") | |
| file(STRINGS configuration.conf PROJECT_CONFIGURATION_FILE) | |
| configure_file( | |
| "${CMAKE_CURRENT_LIST_DIR}/configuration.conf" | |
| "${CMAKE_CURRENT_BINARY_DIR}/configuration.conf" | |
| COPYONLY | |
| ) | |
| file(STRINGS "${CMAKE_CURRENT_BINARY_DIR}/configuration.conf" PROJECT_CONFIGURATION_FILE) |
🤖 Prompt for AI Agents
In cmake/Configure.cmake around lines 20 to 21, the configure_file command uses
a relative path for "configuration.conf" which can break when included from
other directories. Fix this by specifying the full source path using
CMAKE_CURRENT_LIST_DIR for the input file, the full binary directory path for
the output file, and add the COPYONLY option to avoid unintended processing.
This ensures the file is located and copied robustly regardless of the current
working directory.
| set -e | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add pipefail for robust error propagation.
psql | grep | cut … pipelines can mask failures from psql.
-set -e
+set -euo pipefailThis lets the script abort if any pipeline element fails.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set -e | |
| -set -e | |
| +set -euo pipefail |
🤖 Prompt for AI Agents
In tools/scripts/get_signatures.sh at lines 10 to 11, the script uses pipelines
but only has 'set -e' which does not catch failures in pipeline commands
properly. Add 'set -o pipefail' alongside 'set -e' to ensure the script aborts
if any command in a pipeline fails, improving error detection and robustness.
| # DB_ARGS are the remaining of the arguments | ||
| read -ra DB_ARGS <<< "$*" | ||
|
|
||
| FILE="$DIR/pgorpy--$MINOR.sig" | ||
|
|
||
| dropdb --if-exists "${DB_ARGS[@]}" "$DB_NAME" | ||
| createdb "${DB_ARGS[@]}" "$DB_NAME" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure output directory exists before writing.
sql/sigs is not created, so the final redirect fails with “No such file or directory”.
DIR="sql/sigs"
+# create if missing
+mkdir -p "$DIR"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # DB_ARGS are the remaining of the arguments | |
| read -ra DB_ARGS <<< "$*" | |
| FILE="$DIR/pgorpy--$MINOR.sig" | |
| dropdb --if-exists "${DB_ARGS[@]}" "$DB_NAME" | |
| createdb "${DB_ARGS[@]}" "$DB_NAME" | |
| DIR="sql/sigs" | |
| # create if missing | |
| mkdir -p "$DIR" | |
| # DB_ARGS are the remaining of the arguments | |
| read -ra DB_ARGS <<< "$*" | |
| FILE="$DIR/pgorpy--$MINOR.sig" | |
| dropdb --if-exists "${DB_ARGS[@]}" "$DB_NAME" | |
| createdb "${DB_ARGS[@]}" "$DB_NAME" |
🤖 Prompt for AI Agents
In tools/scripts/get_signatures.sh around lines 27 to 33, the script attempts to
write to a file in the sql/sigs directory which may not exist, causing a
failure. Add a command before writing to ensure the output directory (sql/sigs)
exists by creating it if necessary, using mkdir -p or similar, to prevent the
"No such file or directory" error.
| if sys.platform in ('win32', 'win64', 'cygwin'): | ||
| activate_this = os.path.join(venv, 'Scripts', 'activate_this.py') | ||
| else: | ||
| activate_this = os.path.join(venv, 'bin', 'activate_this.py') | ||
|
|
||
| exec(open(activate_this).read(), dict(__file__=activate_this)) | ||
| $BODY$; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Make venv activation robust; handle missing activate_this.py and add a fallback.
activate_this.py isn’t guaranteed to exist in modern venvs. Also, provide clearer errors and a site-packages fallback.
import os
import sys
+ from pathlib import Path
- if sys.platform in ('win32', 'win64', 'cygwin'):
- activate_this = os.path.join(venv, 'Scripts', 'activate_this.py')
+ # Build platform-specific path to activate_this.py (virtualenv)
+ if sys.platform in ('win32', 'cygwin'):
+ activate_this = os.path.join(venv, 'Scripts', 'activate_this.py')
else:
activate_this = os.path.join(venv, 'bin', 'activate_this.py')
- exec(open(activate_this).read(), dict(__file__=activate_this))
+ p = Path(activate_this)
+ if p.is_file():
+ with p.open('r', encoding='utf-8') as fh:
+ code = compile(fh.read(), str(p), 'exec')
+ exec(code, dict(__file__=str(p)))
+ else:
+ # Fallback for stdlib venv: add site-packages to sys.path
+ if sys.platform in ('win32', 'cygwin'):
+ sp = os.path.join(venv, 'Lib', 'site-packages')
+ else:
+ sp = os.path.join(
+ venv, 'lib',
+ f"python{sys.version_info[0]}.{sys.version_info[1]}", 'site-packages'
+ )
+ if os.path.isdir(sp):
+ if sp not in sys.path:
+ sys.path.insert(0, sp)
+ else:
+ raise Exception(
+ f"Could not locate activate_this.py or site-packages under: {venv}"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if sys.platform in ('win32', 'win64', 'cygwin'): | |
| activate_this = os.path.join(venv, 'Scripts', 'activate_this.py') | |
| else: | |
| activate_this = os.path.join(venv, 'bin', 'activate_this.py') | |
| exec(open(activate_this).read(), dict(__file__=activate_this)) | |
| $BODY$; | |
| import os | |
| import sys | |
| from pathlib import Path | |
| # Build platform-specific path to activate_this.py (virtualenv) | |
| if sys.platform in ('win32', 'cygwin'): | |
| activate_this = os.path.join(venv, 'Scripts', 'activate_this.py') | |
| else: | |
| activate_this = os.path.join(venv, 'bin', 'activate_this.py') | |
| p = Path(activate_this) | |
| if p.is_file(): | |
| with p.open('r', encoding='utf-8') as fh: | |
| code = compile(fh.read(), str(p), 'exec') | |
| exec(code, dict(__file__=str(p))) | |
| else: | |
| # Fallback for stdlib venv: add site-packages to sys.path | |
| if sys.platform in ('win32', 'cygwin'): | |
| sp = os.path.join(venv, 'Lib', 'site-packages') | |
| else: | |
| sp = os.path.join( | |
| venv, 'lib', | |
| f"python{sys.version_info[0]}.{sys.version_info[1]}", 'site-packages' | |
| ) | |
| if os.path.isdir(sp): | |
| if sp not in sys.path: | |
| sys.path.insert(0, sp) | |
| else: | |
| raise Exception( | |
| f"Could not locate activate_this.py or site-packages under: {venv}" | |
| ) | |
| $BODY$; |
🤖 Prompt for AI Agents
In tools/testers/sampledata.sql around lines 12 to 18, the code assumes
activate_this.py always exists, which may not be true for modern virtual
environments. Modify the code to check if activate_this.py exists before
executing it; if missing, raise a clear error message. Additionally, add a
fallback mechanism to manually append the site-packages directory of the venv to
sys.path to ensure dependencies are found.
| PGPORT="$1" | ||
| PGDATABASE="$2" | ||
| PGUSER="$3" | ||
| VERSION="$4" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add argument validation and helpful usage.
If VERSION is omitted, current SQL will be invalid. Validate inputs and allow optional VERSION.
PGPORT="$1"
PGDATABASE="$2"
PGUSER="$3"
-VERSION="$4"
+VERSION="${4-}"
+
+if [[ -z "${PGPORT}" || -z "${PGDATABASE}" || -z "${PGUSER}" ]]; then
+ echo "Usage: $0 <pgport> <database> <user> [version]" >&2
+ exit 2
+fi🤖 Prompt for AI Agents
In tools/testers/setup_db.sh around lines 14 to 18, the script assigns
positional parameters to variables without validating them, which can cause
errors if VERSION is omitted. Add input validation to check if the required
arguments PGPORT, PGDATABASE, and PGUSER are provided, and make VERSION optional
with a default value or handle its absence gracefully. Also, include a usage
message that explains the expected arguments and how to run the script
correctly.
| PGOPTIONS='-c client_min_messages=WARNING' psql -U "${PGUSER}" -p "${PGPORT}" -d "${PGDATABASE}" -X -q --set ON_ERROR_STOP=1 --pset pager=off \ | ||
| -c "CREATE EXTENSION IF NOT EXISTS pgtap; CREATE EXTENSION IF NOT EXISTS pgorpy WITH VERSION '${VERSION}' CASCADE;" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Create prerequisite plpython3u and handle optional version safely.
Ensure plpython3u is available before pgorpy. Avoid direct interpolation in SQL when possible; build the command conditionally.
-PGOPTIONS='-c client_min_messages=WARNING' psql -U "${PGUSER}" -p "${PGPORT}" -d "${PGDATABASE}" -X -q --set ON_ERROR_STOP=1 --pset pager=off \
- -c "CREATE EXTENSION IF NOT EXISTS pgtap; CREATE EXTENSION IF NOT EXISTS pgorpy WITH VERSION '${VERSION}' CASCADE;"
+sql_cmds="CREATE EXTENSION IF NOT EXISTS pgtap;
+CREATE EXTENSION IF NOT EXISTS plpython3u;
+"
+if [[ -n "${VERSION}" ]]; then
+ sql_cmds+="CREATE EXTENSION IF NOT EXISTS pgorpy WITH VERSION '${VERSION}' CASCADE;"
+else
+ sql_cmds+="CREATE EXTENSION IF NOT EXISTS pgorpy CASCADE;"
+fi
+PGOPTIONS='-c client_min_messages=WARNING' psql -U "${PGUSER}" -p "${PGPORT}" -d "${PGDATABASE}" -X -q --set ON_ERROR_STOP=1 --pset pager=off \
+ -c "${sql_cmds}"Please confirm pgorpy requires plpython3u in CREATE EXTENSION context; otherwise, we can keep it as a precondition step in the test harness.
🏁 Script executed:
#!/bin/bash
echo "=== Searching for pgorpy control file ==="
rg -A2 "name *= *'pgorpy'" -g "*.control" || echo "No pgorpy control file found"
echo "=== Searching for plpython3u references ==="
rg -n "plpython3u" || echo "No plpython3u references found"Length of output: 1259
Simplify extension setup and guard against empty VERSION
The pgorpy.control file already declares requires = 'plpython3u', and you’re using CASCADE, so PostgreSQL will automatically install plpython3u when creating pgorpy. You can therefore remove the explicit CREATE EXTENSION plpython3u; step. However, it’s still prudent to avoid interpolating an empty VERSION string into your SQL.
• Remove the explicit plpython3u creation
• Add conditional logic around ${VERSION} to prevent WITH VERSION ''
Suggested diff:
-PGOPTIONS='-c client_min_messages=WARNING' psql -U "${PGUSER}" -p "${PGPORT}" \
- -d "${PGDATABASE}" -X -q --set ON_ERROR_STOP=1 --pset pager=off \
- -c "CREATE EXTENSION IF NOT EXISTS pgtap; CREATE EXTENSION IF NOT EXISTS pgorpy WITH VERSION '${VERSION}' CASCADE;"
+sql="CREATE EXTENSION IF NOT EXISTS pgtap;"
+if [[ -n "${VERSION}" ]]; then
+ sql+=" CREATE EXTENSION IF NOT EXISTS pgorpy WITH VERSION '${VERSION}' CASCADE;"
+else
+ sql+=" CREATE EXTENSION IF NOT EXISTS pgorpy CASCADE;"
+fi
+
+PGOPTIONS='-c client_min_messages=WARNING' psql \
+ -U "${PGUSER}" -p "${PGPORT}" -d "${PGDATABASE}" \
+ -X -q --set ON_ERROR_STOP=1 --pset pager=off \
+ -c "${sql}"🤖 Prompt for AI Agents
In tools/testers/setup_db.sh around lines 19 to 21, remove the explicit creation
of the plpython3u extension since it is automatically installed via CASCADE when
creating pgorpy. Also, add conditional logic to check if the VERSION variable is
non-empty before including the WITH VERSION clause in the CREATE EXTENSION
command to avoid interpolating an empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review continued from previous batch...
| my ($key) = substr($line, 4, index(substr($line, 4), "|")); | ||
| my ($value) = substr($line, index($line,"`")); | ||
| $value =~ s/\R//g; | ||
| $data{$key} = $value; | ||
| print "$key $data{$key} \n" if $DEBUG; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fragile substr … index … parsing can throw on lines without a pipe
If a line under rst_epilog lacks a |, index returns -1 and substr raises “substr outside string”, aborting the script.
Replace the manual slicing with a safe regex:
if ($line =~ /^\s*\|\s*(.+?)\|\s*`(.+)`/) {
my ($key, $value) = ($1, $2);
$data{$key} = $value;
}This avoids negative indices and is easier to read.
🤖 Prompt for AI Agents
In .github/scripts/notes2news.pl around lines 185 to 189, the current parsing
uses substr and index which can fail with "substr outside string" if the line
lacks a pipe character. Replace this fragile manual slicing with a regex that
safely captures the key and value from lines matching the pattern with pipes and
backticks. Use a regex like /^\s*\|\s*(.+?)\|\s*`(.+)`/ to extract key and
value, then assign them to the data hash only if the line matches, preventing
errors and improving readability.
| for s1 in "${SIGNATURES[@]}" | ||
| do | ||
| for s2 in "${SIGNATURES[@]}" | ||
| do | ||
| # only comparing lower version with higher version | ||
| if (( $(echo "$s1 >= $s2" | bc -l) )); then continue; fi | ||
|
|
||
| mayor1=$(echo "$s1" | perl -pe 's/([0-9]+).*/$1/') | ||
| mayor2=$(echo "$s2" | perl -pe 's/([0-9]+).*/$1/') | ||
|
|
||
| # comparing within same mayors only | ||
| if [ "$mayor1" != "$mayor2" ]; then continue; fi | ||
|
|
||
| # ignoring any signature changes made on v0 | ||
| if [ "$mayor1" == 0 ]; then continue; fi | ||
|
|
||
| missing+=$(comm -23 "pgorpy--$s1.sig" "pgorpy--$s2.sig") | ||
| done | ||
| done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version comparison is incorrect; bc cannot compare dot-separated versions.
Using bc -l on strings like 1.2.3 is invalid. Also, comm expects sorted inputs.
Refactor to sort versions with sort -V, iterate in order, and sort files before comm:
-mapfile -t SIGNATURES < <(git ls-files "*.sig" | perl -pe 's/(.*)--(.*)\.sig/$2/')
-
-for s1 in "${SIGNATURES[@]}"
-do
- for s2 in "${SIGNATURES[@]}"
- do
- # only comparing lower version with higher version
- if (( $(echo "$s1 >= $s2" | bc -l) )); then continue; fi
-
- mayor1=$(echo "$s1" | perl -pe 's/([0-9]+).*/$1/')
- mayor2=$(echo "$s2" | perl -pe 's/([0-9]+).*/$1/')
-
- # comparing within same mayors only
- if [ "$mayor1" != "$mayor2" ]; then continue; fi
-
- # ignoring any signature changes made on v0
- if [ "$mayor1" == 0 ]; then continue; fi
-
- missing+=$(comm -23 "pgorpy--$s1.sig" "pgorpy--$s2.sig")
- done
-done
+mapfile -t SIGNATURES < <(git ls-files "*.sig" | sed -E 's|.*/pgorpy--(.*)\.sig|\1|' | sort -V)
+
+for ((i=0; i<${#SIGNATURES[@]}; i++)); do
+ s1="${SIGNATURES[$i]}"
+ major1="${s1%%.*}"
+ # ignore any signature changes made on v0
+ [[ "$major1" == "0" ]] && continue
+ for ((j=i+1; j<${#SIGNATURES[@]}; j++)); do
+ s2="${SIGNATURES[$j]}"
+ major2="${s2%%.*}"
+ # compare only within same major
+ [[ "$major1" != "$major2" ]] && continue
+ missing+=$(comm -23 <(sort "pgorpy--$s1.sig") <(sort "pgorpy--$s2.sig"))
+ done
+done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for s1 in "${SIGNATURES[@]}" | |
| do | |
| for s2 in "${SIGNATURES[@]}" | |
| do | |
| # only comparing lower version with higher version | |
| if (( $(echo "$s1 >= $s2" | bc -l) )); then continue; fi | |
| mayor1=$(echo "$s1" | perl -pe 's/([0-9]+).*/$1/') | |
| mayor2=$(echo "$s2" | perl -pe 's/([0-9]+).*/$1/') | |
| # comparing within same mayors only | |
| if [ "$mayor1" != "$mayor2" ]; then continue; fi | |
| # ignoring any signature changes made on v0 | |
| if [ "$mayor1" == 0 ]; then continue; fi | |
| missing+=$(comm -23 "pgorpy--$s1.sig" "pgorpy--$s2.sig") | |
| done | |
| done | |
| mapfile -t SIGNATURES < <(git ls-files "*.sig" \ | |
| | sed -E 's|.*/pgorpy--(.*)\.sig|\1|' \ | |
| | sort -V) | |
| for ((i=0; i<${#SIGNATURES[@]}; i++)); do | |
| s1="${SIGNATURES[$i]}" | |
| major1="${s1%%.*}" | |
| # ignore any signature changes made on v0 | |
| [[ "$major1" == "0" ]] && continue | |
| for ((j=i+1; j<${#SIGNATURES[@]}; j++)); do | |
| s2="${SIGNATURES[$j]}" | |
| major2="${s2%%.*}" | |
| # compare only within same major | |
| [[ "$major1" != "$major2" ]] && continue | |
| missing+=$(comm -23 \ | |
| <(sort "pgorpy--$s1.sig") \ | |
| <(sort "pgorpy--$s2.sig")) | |
| done | |
| done |
🤖 Prompt for AI Agents
In .github/scripts/test_signatures.sh around lines 13 to 31, the version
comparison using bc -l is invalid for dot-separated versions like 1.2.3, and
comm requires sorted inputs. Refactor the script to first sort the SIGNATURES
array using sort -V to ensure proper version ordering, then iterate through the
versions in ascending order to avoid invalid comparisons. Additionally, before
using comm, sort the signature files to guarantee correct input order for comm.
| #!/bin/bash | ||
| # ------------------------------------------------------------------------------ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add strict-mode guards
Enable set -euo pipefail right after the shebang so the script aborts on any failing command or unset variable, preventing partial commits in CI.
#!/bin/bash
+set -euo pipefail📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/bin/bash | |
| # ------------------------------------------------------------------------------ | |
| #!/bin/bash | |
| set -euo pipefail | |
| # ------------------------------------------------------------------------------ |
🤖 Prompt for AI Agents
In .github/scripts/update_locale.sh at the beginning of the file (lines 1-2),
add the line `set -euo pipefail` immediately after the shebang (`#!/bin/bash`).
This will enable strict mode to ensure the script exits on any error, unset
variable, or failed pipeline, preventing partial commits during CI runs.
| tags: [] | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Remove empty tags field (actionlint warning)
tags: [] is unnecessary and flagged by actionlint. Remove it.
- tags: []🏁 Script executed:
#!/bin/bash
# Display occurrences of 'tags:' in the documentation workflow
rg -n 'tags:' -C2 .github/workflows/documentation.yml || trueLength of output: 118
🏁 Script executed:
#!/bin/bash
# Show the first 50 lines of the documentation workflow for context
head -n50 .github/workflows/documentation.yml || trueLength of output: 1018
Remove empty tags filter in documentation workflow
The empty tags: [] entry in the push trigger is flagged by actionlint and can be safely removed to restore the default behavior (trigger on all tags) without warnings.
• File: .github/workflows/documentation.yml
Line 25
Suggested diff:
- tags: []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tags: [] |
🧰 Tools
🪛 actionlint (1.7.7)
25-25: "tags" section should not be empty
(syntax-check)
🤖 Prompt for AI Agents
In .github/workflows/documentation.yml at line 25, remove the empty tags: []
filter from the push trigger. This empty array is unnecessary and causes
actionlint warnings. Deleting this line will restore the default behavior of
triggering on all tags without any filter.
| - name: Install Google OR-Tools | ||
| if: env.PROCESS == 'true' | ||
| run: | | ||
| sudo pip install --root=/ ortools==9.10.4067 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid system-wide sudo pip for ortools
Install packages in the runner user context or a venv.
- - name: Install Google OR-Tools
- if: env.PROCESS == 'true'
- run: |
- sudo pip install --root=/ ortools==9.10.4067
+ - name: Install Google OR-Tools
+ if: env.PROCESS == 'true'
+ run: |
+ python -m pip install --upgrade pip
+ pip install ortools==9.10.4067Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/documentation.yml around lines 111 to 115, avoid using
'sudo pip install' for ortools as it installs packages system-wide. Instead,
modify the workflow to install ortools within the runner user's context by
either using a Python virtual environment or installing without sudo to keep the
environment isolated and prevent permission issues.
| CREATE OR REPLACE FUNCTION function_types_has(TEXT, TEXT) | ||
| RETURNS TEXT AS | ||
| $BODY$ | ||
| SELECT set_has(format($$ | ||
| WITH | ||
| a AS ( | ||
| SELECT oid, u.name, u.idx | ||
| FROM pg_proc p CROSS JOIN unnest(p.proallargtypes) | ||
| WITH ordinality as u(name, idx) | ||
| WHERE p.proname = '%1$s'), | ||
| b AS ( | ||
| SELECT a.*, t.typname FROM a JOIN pg_type As t on (t.oid = a.name)) | ||
| SELECT array_agg(typname ORDER BY idx) FROM b GROUP BY oid | ||
| $$, $1), $2, $1 || ': Function types'); | ||
| $BODY$ LANGUAGE SQL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Handle NULL proallargtypes in function_types_has
Use the same coalesce(p.proallargtypes, p.proargtypes) approach as in function_types_eq to avoid NULLs.
- FROM pg_proc p CROSS JOIN unnest(p.proallargtypes)
+ FROM pg_proc p CROSS JOIN unnest(coalesce(p.proallargtypes, p.proargtypes))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE OR REPLACE FUNCTION function_types_has(TEXT, TEXT) | |
| RETURNS TEXT AS | |
| $BODY$ | |
| SELECT set_has(format($$ | |
| WITH | |
| a AS ( | |
| SELECT oid, u.name, u.idx | |
| FROM pg_proc p CROSS JOIN unnest(p.proallargtypes) | |
| WITH ordinality as u(name, idx) | |
| WHERE p.proname = '%1$s'), | |
| b AS ( | |
| SELECT a.*, t.typname FROM a JOIN pg_type As t on (t.oid = a.name)) | |
| SELECT array_agg(typname ORDER BY idx) FROM b GROUP BY oid | |
| $$, $1), $2, $1 || ': Function types'); | |
| $BODY$ LANGUAGE SQL; | |
| CREATE OR REPLACE FUNCTION function_types_has(TEXT, TEXT) | |
| RETURNS TEXT AS | |
| $BODY$ | |
| SELECT set_has(format($$ | |
| WITH | |
| a AS ( | |
| SELECT oid, u.name, u.idx | |
| FROM pg_proc p CROSS JOIN unnest(coalesce(p.proallargtypes, p.proargtypes)) | |
| WITH ordinality as u(name, idx) | |
| WHERE p.proname = '%1$s'), | |
| b AS ( | |
| SELECT a.*, t.typname FROM a JOIN pg_type As t on (t.oid = a.name)) | |
| SELECT array_agg(typname ORDER BY idx) FROM b GROUP BY oid | |
| $$, $1), $2, $1 || ': Function types'); | |
| $BODY$ LANGUAGE SQL; |
🤖 Prompt for AI Agents
In tools/testers/general_pgtap_tests.sql around lines 55 to 69, the function
function_types_has uses p.proallargtypes which can be NULL, causing issues.
Modify the query to use coalesce(p.proallargtypes, p.proargtypes) instead of
just p.proallargtypes to handle NULL values safely, matching the approach used
in function_types_eq.
| FOR i IN 0..array_length(params, 1) LOOP | ||
| separator = ' '; | ||
| mp := params; | ||
| IF i != 0 THEN | ||
| mp[i] = subs[i]; | ||
| END IF; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off-by-one array indexing will raise “subscript out of bounds”.
PostgreSQL arrays default to 1-based indexes, yet the loop starts at 0 and later dereferences error_messages[i], subs[i], etc. Accessing element 0 will fail at runtime.
-FOR i IN 0..array_length(params, 1) LOOP
+FOR i IN 1..coalesce(array_length(params, 1),0) LOOPAdjust subsequent checks accordingly (drop the i != 0 guard).
Also applies to: 39-47
🤖 Prompt for AI Agents
In tools/testers/no_crash_test.sql around lines 20 to 26, the loop iterates from
0 to array_length but PostgreSQL arrays are 1-based, causing out-of-bounds
errors when accessing index 0. Change the loop to start from 1 instead of 0 and
remove the condition checking for i != 0. Apply the same fix to the loop on
lines 39 to 47 to ensure all array accesses use valid 1-based indexes.
| RETURN query SELECT * FROM lives_ok(q1, 'lives_ok:' || q1); | ||
| IF i = ANY(non_empty_args) THEN | ||
| RETURN query SELECT * FROM lives_ok(q1, 'lives_ok: ' || q1); | ||
| ELSE | ||
| RETURN query SELECT * FROM is_empty(q1, 'is_empty: ' || q1); | ||
| END IF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Duplicate lives_ok / unreachable branch logic.
The first RETURN QUERY SELECT * FROM lives_ok(...) already appends a result; the nested IF i = ANY(non_empty_args) that follows is dead for the same execution path and never reached. Consolidate the conditional to avoid double execution and improve clarity.
🤖 Prompt for AI Agents
In tools/testers/no_crash_test.sql around lines 42 to 47, the first RETURN QUERY
SELECT * FROM lives_ok(...) executes unconditionally, making the subsequent IF
block with another RETURN QUERY unreachable and redundant. Remove the initial
unconditional RETURN QUERY and restructure the code to use a single conditional
that returns from lives_ok or is_empty based on whether i is in non_empty_args,
ensuring only one RETURN QUERY executes per call.
| VERSION=$(grep pgORpy CMakeLists.txt | awk '{print $3}') | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Fragile version extraction from CMakeLists.txt.
The current grep/awk may grab the wrong token if multiple matches exist. Parse the project line explicitly.
-VERSION=$(grep pgORpy CMakeLists.txt | awk '{print $3}')
+# Expect a line like: project(pgorpy VERSION 0.0.0)
+VERSION=$(awk '
+ BEGIN{IGNORECASE=1}
+ $0 ~ /^project\(/ && $0 ~ /pgorpy/ && $0 ~ /version/ {
+ for (i=1;i<=NF;i++) if (tolower($i)=="version") { print $(i+1); exit }
+ }
+' CMakeLists.txt)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| VERSION=$(grep pgORpy CMakeLists.txt | awk '{print $3}') | |
| # Expect a line like: project(pgorpy VERSION 0.0.0) | |
| VERSION=$(awk ' | |
| BEGIN { IGNORECASE = 1 } | |
| $0 ~ /^project\(/ && $0 ~ /pgorpy/ && $0 ~ /version/ { | |
| for (i = 1; i <= NF; i++) | |
| if (tolower($i) == "version") { | |
| print $(i+1) | |
| exit | |
| } | |
| } | |
| ' CMakeLists.txt) |
🤖 Prompt for AI Agents
In tools/testers/pg_prove_tests.sh at lines 13-14, the current method using grep
and awk to extract the version from CMakeLists.txt is fragile and may capture
incorrect tokens if multiple matches exist. Modify the script to explicitly
parse the project version line by searching for the exact line defining the
project version and extracting the version string reliably, for example by using
a more precise pattern match or a dedicated parsing tool to avoid ambiguity.
| pushd ./tools/testers/ > /dev/null || exit 1 | ||
| bash setup_db.sh "${PGPORT[1]}" "${PGDATABASE}" "${PGUSER[1]}" "${VERSION}" | ||
| popd > /dev/null || exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Assumes repo root as CWD; make setup_db.sh invocation location-agnostic.
Use the script’s directory to locate setup_db.sh and avoid relying on where the user runs the script.
-pushd ./tools/testers/ > /dev/null || exit 1
-bash setup_db.sh "${PGPORT[1]}" "${PGDATABASE}" "${PGUSER[1]}" "${VERSION}"
-popd > /dev/null || exit 1
+# Resolve the directory of this script
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+bash "${SCRIPT_DIR}/setup_db.sh" "${PGPORT[1]}" "${PGDATABASE}" "${PGUSER[1]}" "${VERSION}"🤖 Prompt for AI Agents
In tools/testers/pg_prove_tests.sh around lines 58 to 60, the script assumes the
current working directory is the repo root when invoking setup_db.sh, which can
cause failures if run from other locations. Modify the script to determine the
directory of the current script and use that path to call setup_db.sh, making
the invocation location-agnostic and ensuring it works regardless of where the
script is executed from.
|
@coderabbitai create an issue with a summary of your suggestions |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1388 🏁 Script executed: Length of output: 633 @cvvergara, I've successfully created issue #5 with a comprehensive summary of the code review suggestions for this PR. The issue includes:
You can review the full issue here: #5 The issue is organized with clear action items that can be addressed either in this PR or tracked for future improvements. Feel free to update the issue or let me know if you'd like any adjustments to the summary! |
Fixes #1
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit
New Features
por_version,por_full_version) for extension and OR-Tools version info.Bug Fixes
Documentation
Chores
Tests
Style
Refactor
Revert