Hiroshima up to basic vehicles#280
Conversation
- cleanup index
Matches ubuntu.yml — Ubuntu apt only provides osm2pgrouting 2.3.8, workshop now requires 3.0.0.
FOLDER_NAME on main branch now matches PGR_WORKSHOP_VERSION (3.2).
WalkthroughThe PR switches the workshop from Auckland 3.1 to Hiroshima 3.2, rebuilds OSGeoLive installation and data setup, rewrites chapter text and generated SQL/views for the basic and SDG materials, and refreshes the English, POT, Spanish, and Swedish catalogs. ChangesHiroshima workshop migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
docs/un_sdg/sdg7-energy.rst (3)
186-186: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix incorrect literalinclude path:
sdg3→sdg7.Line 186 references
../scripts/un_sdg/sdg3/only_connected8.txtbut should reference../scripts/un_sdg/sdg7/only_connected8.txt. Including SDG 3 output in the SDG 7 chapter will display incorrect command results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/un_sdg/sdg7-energy.rst` at line 186, The literalinclude in the SDG 7 documentation is pointing at the wrong source under sdg3 instead of sdg7. Update the directive in sdg7-energy.rst to reference the sdg7 version of only_connected8.txt so the SDG 7 chapter uses the correct script output.
175-175: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix incorrect literalinclude path:
sdg3→sdg7.Line 175 references
../scripts/un_sdg/sdg3/only_connected7.txtbut should reference../scripts/un_sdg/sdg7/only_connected7.txt. Including SDG 3 output in the SDG 7 chapter will display incorrect command results to workshop attendees.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/un_sdg/sdg7-energy.rst` at line 175, The sdg7 documentation is including the wrong script output via the literalinclude directive. Update the include target in the sdg7-energy.rst chapter from the sdg3 path to the matching sdg7 file so the SDG 7 section references the correct `only_connected7.txt` example.
8-8: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix incorrect SDG goal number.
"Sustainable Development Goal 11" should be "Sustainable Development Goal 7" for the Affordable and Clean Energy chapter. This appears to be a copy-paste error from the SDG 11 chapter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/un_sdg/sdg7-energy.rst` at line 8, Fix the incorrect Sustainable Development Goal reference in the Affordable and Clean Energy chapter so it matches the chapter’s subject. Update the sentence in the SDG7 energy documentation from “Sustainable Development Goal 11” to “Sustainable Development Goal 7,” keeping the surrounding wording intact and ensuring the chapter header text remains consistent with the `sdg7-energy` content.locale/es/LC_MESSAGES/un_sdg/data.po (1)
209-215: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the appendix title localized in the Spanish catalog.
Switching both the heading and the inline reference to
Appendixregresses the Spanish docs. If the anchor needs to stay stable, add an explicit label in the source.rstinstead of forcing the English title intomsgstr.Also applies to: 367-369
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/es/LC_MESSAGES/un_sdg/data.po` around lines 209 - 215, The Spanish translation for the Appendix reference was incorrectly switched back to English, so keep the localized title in the `msgstr` for the affected `data.po` entries. Update the translations around the `Appendix` reference to preserve the Spanish heading, and if the anchor must remain stable, adjust the source `.rst` to add an explicit label rather than hardcoding the English term in the catalog. Apply the same fix to the other matching `msgid`/`msgstr` pair mentioned in the comment.locale/sv/LC_MESSAGES/basic/pedestrian.po (1)
146-150: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThis PO needs a full msgmerge against the new Hiroshima template.
Only updating placeholders here is not enough.
locale/pot/basic/pedestrian.potwas rewritten for Hiroshima 3.2, while this Swedish catalog still carries the old Belém-era header and old msgids/content. Because translation lookup is msgid-based, most of this file will no longer match the current template and Swedish readers will fall back to English for large parts of the chapter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/sv/LC_MESSAGES/basic/pedestrian.po` around lines 146 - 150, This Swedish catalog needs a full msgmerge with the updated Hiroshima 3.2 template, not just placeholder edits. Regenerate `locale/sv/LC_MESSAGES/basic/pedestrian.po` from the current `locale/pot/basic/pedestrian.pot` so the old Belém-era header and outdated msgids are replaced with the new template entries. Keep the existing translations where they still match after merge, and resolve any fuzzy or obsolete entries introduced by the rewrite.docs/basic/pedestrian.rst (1)
282-295: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExercise 5 problem/solution mismatch
The Exercise 5 problem statement and solution describe inconsistent origins/destinations:
Problem (lines 282-286):
- First pedestrian: from |place_1| to |place_4|
- Second pedestrian: from |place_3| to |place_5|
Solution (lines 294-295):
- First pedestrian: from |id_1| to |id_3| (i.e., |place_1| to |place_3|)
- Second pedestrian: from |id_2| to |id_5| (i.e., |place_2| to |place_5|)
Neither pedestrian's route matches between problem and solution. Please align both sections to the same identifiers.
📝 Suggested fix (if problem text is correct)
- * First pedestrian departs from |id_1| and the destination is |id_3|. - * Second pedestrian departs from |id_2| and the destination is |id_5|. + * First pedestrian departs from |id_1| and the destination is |id_4|. + * Second pedestrian departs from |id_3| and the destination is |id_5|.Or if the solution is correct and the problem should be updated:
- * First pedestrian goes from "|place_1|" to "|place_4|" - * Second pedestrian goes from "|place_3|" to "|place_5|" + * First pedestrian goes from "|place_1|" to "|place_3|" + * Second pedestrian goes from "|place_2|" to "|place_5|"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/basic/pedestrian.rst` around lines 282 - 295, Align the Exercise 5 statement and the rubric:: Solution in pedestrian.rst so they use the same origin/destination pairs; the current problem text uses place_1→place_4 and place_3→place_5, while the solution in the same section refers to id_1→id_3 and id_2→id_5. Update either the problem bullets or the solution bullets (in the Walking exercise block and Solution block) so the identifiers and route endpoints match exactly.docs/scripts/basic/vehicles/CMakeLists.txt (1)
1-32: 🩺 Stability & Availability | 🟡 MinorRestore
images.sqlin the vehicles target
docs/scripts/basic/vehicles/images.sqlis not run by any target indocs/scripts/basic/vehicles; the only command there executesvehicles.sql. If the_pnghelper views are still needed, addimages.sqlback into this target.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/scripts/basic/vehicles/CMakeLists.txt` around lines 1 - 32, The basic_vehicles_scripts target only runs vehicles.sql, so the images.sql script is currently omitted from this CMake target. Update the basic_vehicles_scripts setup in CMakeLists.txt to include images.sql alongside vehicles.sql if the _png helper views are still required, using the existing add_custom_command/configure_file pattern so the target generates the missing outputs.
🟡 Minor comments (36)
docs/general-intro/overview.rst-33-34 (1)
33-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo and duplicated phrase in first bullet.
Line 33 has "PostreSQL" (should be "PostgreSQL"). Line 34 repeats "can be modified" twice.
📝 Proposed fix
-* Data and attributes are stored on a PostreSQL database and as such they can be - modified can be modified by many clients. +* Data and attributes are stored on a PostgreSQL database and as such they can be + modified by many clients.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/general-intro/overview.rst` around lines 33 - 34, Fix the first bullet in the overview text by correcting the database name in the PostgreSQL sentence and removing the duplicated “can be modified” phrase. Update the wording in the bullet under the introductory list so it reads cleanly and accurately, keeping the change limited to that sentence.docs/general-intro/overview.rst-42-44 (1)
42-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix malformed RST hyperlink and typo in feature list.
Line 42 has a stray backtick inside the link text that breaks the hyperlink:
`Functions based on `A* Search Algorithm. Line 44 has "commponent" (should be "component").📝 Proposed fix
-* `Functions based on `A* Search Algorithm <https://docs.pgrouting.org/latest/en/aStar-family.html>`__ -* `Graph commponent functions <https://docs.pgrouting.org/latest/en/components-family.html>`__ +* `Functions based on A* Search Algorithm <https://docs.pgrouting.org/latest/en/aStar-family.html>`__ +* `Graph component functions <https://docs.pgrouting.org/latest/en/components-family.html>`__🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/general-intro/overview.rst` around lines 42 - 44, Fix the malformed reStructuredText link in the feature list by removing the stray backtick in the A* item so the link text is valid, and correct the typo in the components entry from “commponent” to “component”. Update the list entries in overview.rst where the Dijkstra/A* and graph component bullets are defined.docs/basic/data.rst-71-79 (1)
71-79: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe download note still points readers to the wrong source.
This section now pulls the workshop snapshot from the pgRouting workshop download path, but the next sentence still says the identical file is on the OSGeoLive download page. That no longer matches the Hiroshima setup and can send attendees to the wrong place.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/basic/data.rst` around lines 71 - 79, The download note in the data section is still pointing readers to the old OSGeoLive source instead of the workshop snapshot path used by the current setup. Update the sentence near the literalinclude in the data document so it references the pgRouting workshop download location consistently, using the surrounding “Download data” text and the included get_all_data.sh snippet as the anchor.locale/en/LC_MESSAGES/basic/data.po-76-102 (1)
76-102: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the new English typos before regenerating this catalog.
This chunk introduces user-facing errors such as
otpionallyandDownload data form pgRouting download. Please correct them in the source.rstand regenerate, otherwise they become the English docs text and translator source strings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/en/LC_MESSAGES/basic/data.po` around lines 76 - 102, The new English source strings contain typos that should be fixed before regenerating the catalog. Update the affected text in the documentation source used by these entries, especially the strings for “Install pgRouting and its requirements...” and “Download data form pgRouting download,” so the typoed words are corrected in the source .rst before running the catalog regeneration. Refer to the corresponding documentation text in the workshop data section and ensure the regenerated PO entries pick up the corrected source strings.locale/en/LC_MESSAGES/basic/data.po-12-12 (1)
12-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRegenerate the English catalog header from the Hiroshima metadata.
Only
POT-Creation-Datewas updated here; the header still identifies this file asWorkshop FOSS4G Belém 3.0/Belém package. Since thelocale/en/LC_MESSAGEStree is the source side for translation sync, stale metadata here propagates the wrong workshop/version context downstream.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/en/LC_MESSAGES/basic/data.po` at line 12, The English catalog header is stale and still reflects the old workshop/package metadata; regenerate the header from the Hiroshima source metadata so the locale/en/LC_MESSAGES/basic/data.po entry uses the correct current project identity. Update the catalog header fields in the .po file consistently, not just POT-Creation-Date, and keep the existing gettext header structure intact.locale/en/LC_MESSAGES/appendix/osgeolive.po-225-226 (1)
225-226: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the example ISO name to match the rest of the chapter.
Line 226 still shows
osgeolive-10.0alpha3-amd64.iso, which conflicts with the surrounding OSGeoLive 17 instructions and will mislead attendees following the setup steps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/en/LC_MESSAGES/appendix/osgeolive.po` around lines 225 - 226, The example ISO name in the translation entry is inconsistent with the OSGeoLive 17 setup instructions and should be updated to match the rest of the chapter. In the osgeolive.po entry for the optical drive example, replace the outdated osgeolive-10.0alpha3-amd64.iso reference with the current OSGeoLive 17 ISO naming used elsewhere in the appendix so the example stays aligned and avoids confusing readers.locale/en/LC_MESSAGES/basic/pedestrian.po-54-58 (1)
54-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the new English typos before regenerating this catalog.
This update introduces visible source-string errors like
oms2pgrouting,Don'f, andsanme. Please fix them indocs/basic/pedestrian.rstfirst; patching the generated.poalone will not stick.Also applies to: 87-88, 436-438
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/en/LC_MESSAGES/basic/pedestrian.po` around lines 54 - 58, The English source text contains typos that are being propagated into the generated catalog, so fix the original wording in docs/basic/pedestrian.rst before regenerating the translation files. Update the affected strings around the pedestrian exercise content, including the incorrect identifier text and the misspellings like Don'f and sanme, then rerun the catalog generation so basic/pedestrian.po is refreshed from the corrected source.locale/sv/LC_MESSAGES/basic/graphs.po-517-519 (1)
517-519: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis cross-reference no longer matches the Swedish heading.
The target section is still translated here as
Övning 5: Skapa en vy för routing, so pointing this reference at the English title is likely to break the Swedish link. Keep the localized title, or switch the source doc to an explicit label.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/sv/LC_MESSAGES/basic/graphs.po` around lines 517 - 519, The cross-reference in the Swedish translation is pointing to the English exercise title instead of the localized section name, so update the msgstr in the graphs.po entry to reference the Swedish heading used by the target section, or align the source docs by adding a stable explicit label in the docs/graphs reference and then use that label consistently. Locate the translation entry for the “Similar to Exercise 5: Creating a view for routing” text and make sure the reference matches the translated heading name.locale/en/LC_MESSAGES/general-intro/introduction.po-105-107 (1)
105-107: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the section heading spelling.
Aknowledmentsis user-visible and will propagate to generated docs and translations. Please fix the source heading toAcknowledgmentsand regenerate the catalog.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/en/LC_MESSAGES/general-intro/introduction.po` around lines 105 - 107, The section heading text is misspelled in the translation source and will surface in generated docs, so update the source heading referenced by the introduction catalog from Aknowledments to Acknowledgments and regenerate the locale catalog so the msgid stays in sync.locale/pot/appendix/osgeolive.pot-199-202 (1)
199-202: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the ISO filename example to match OSGeoLive 17.
Earlier steps tell attendees to download OSGeoLive 17, but this later screen text still shows
osgeolive-10.0alpha3-amd64.iso. That mismatch is likely to confuse readers following the install flow. Please update the source appendix and regenerate the POT.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/pot/appendix/osgeolive.pot` around lines 199 - 202, The ISO filename example in the OSGeoLive appendix is outdated and still references an old release name. Update the source text used by the appendix entry so the example matches OSGeoLive 17, then regenerate the POT so the msgid in osgeolive.pot reflects the new filename; use the appendix content that feeds this translation entry to locate the change.locale/en/LC_MESSAGES/basic/vehicle.po-266-268 (1)
266-268: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the exercise title typo upstream.
penalaziedis now part of the English source text, so it will leak into generated docs and future translations. Correct it in the source.rstand regenerate this catalog.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/en/LC_MESSAGES/basic/vehicle.po` around lines 266 - 268, The exercise title in the translation catalog contains a source-text typo that should be fixed upstream, so update the originating wording in the vehicle exercise content and regenerate this PO file. Locate the entry tied to the Exercise 4 title in the vehicle docs source and correct the misspelling before rebuilding the locale catalog so the translated strings stay in sync.locale/en/LC_MESSAGES/basic/vehicle.po-10-12 (1)
10-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRegenerate the English catalog headers from the Hiroshima metadata.
This regenerated file still advertises
Workshop FOSS4G Belém 3.0, while the new POTs in this PR use Hiroshima 3.2. That mixed metadata usually means the locale generation step is still pulling the old template. Please fix the source/generation path and re-export the English catalogs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/en/LC_MESSAGES/basic/vehicle.po` around lines 10 - 12, The English catalog header is still using the old Workshop FOSS4G Belém 3.0 metadata instead of the Hiroshima 3.2 template. Update the locale generation/source path so the catalog is regenerated from the new Hiroshima POT metadata, then re-export the English catalogs to ensure the header fields in the basic/vehicle.po output match the current template.locale/sv/LC_MESSAGES/basic/withPoints.po-523-523 (1)
523-523: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThese cross-references should match the translated Swedish section titles.
Both references were switched to English, while the actual target headings in this catalog remain translated (
Övning 3,Övning 4). That breaks the implicit link contract for the Swedish build. Keep the localized titles inmsgstr, or use explicit labels in the source rst to make the links locale-agnostic.Also applies to: 578-578
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/sv/LC_MESSAGES/basic/withPoints.po` at line 523, The Swedish cross-reference text in the affected msgstr entries uses English section titles instead of the localized headings, which breaks the link target contract for this catalog. Update the msgstr content in basic/withPoints.po so the references match the Swedish titles used by the corresponding reStructuredText sections (for example the localized “Övning” headings), or switch the source links to explicit labels so they stay locale-agnostic. Keep the fix consistent across both affected msgstr entries in this file.locale/en/LC_MESSAGES/general-intro/overview.po-56-94 (1)
56-94: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winProofread this overview block before regenerating translations.
This chunk has several user-facing regressions:
PostreSQL, duplicatedcan be modified, malformed A* link markup, andcommponent. Because this is the English source catalog, these mistakes will flow into the docs and downstream locales unless they are fixed in the source.rst.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/en/LC_MESSAGES/general-intro/overview.po` around lines 56 - 94, This overview block contains source text typos and malformed markup that should be corrected before translations are regenerated. Update the original overview content referenced by these msgids to fix “PostgreSQL”, remove the duplicated “can be modified”, correct the A* link markup so the inline link renders properly, and fix “commponent” to “component”. Keep the changes in the source text behind these entries so the catalog and downstream locales pick up the cleaned wording.locale/sv/LC_MESSAGES/basic/vehicle.po-241-241 (1)
241-241: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep these implicit section references aligned with the Swedish headings.
These
msgstrvalues now point to English section titles, but the targets in this catalog are still translated (Övning 2,Övning 3,Övning 5). With implicit title-based references, that change makes the Swedish links fragile or broken. Keep the localized titles here, or switch the source rst to explicit labels.Also applies to: 438-438, 483-483
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/sv/LC_MESSAGES/basic/vehicle.po` at line 241, The Swedish msgstr entries for the implicit section links have been changed to English titles, which breaks alignment with the translated headings. Update the affected translation strings in the vehicle catalog so they keep the localized section names (for example the Swedish exercise titles) that match the existing targets, or change the source reStructuredText to explicit labels so the references are no longer title-dependent. Apply this to the linked msgstr entries that currently point at the exercise sections.locale/pot/basic/data.pot-67-100 (1)
67-100: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the new template typos before regenerating catalogs.
This block still has a few user-facing mistakes, e.g.
otpionallyandDownload data form .... Those errors will propagate into the English docs and every downstream translation update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/pot/basic/data.pot` around lines 67 - 100, Fix the user-facing typos in the translation template before regenerating catalogs. In the messages near the workshop data section, correct the misspellings in the strings like “otpionally” and “Download data form pgRouting download” so the source text is clean. Update the affected msgid entries in basic data template content so future docs and downstream translations pick up the corrected wording.locale/pot/basic/graphs.pot-256-266 (1)
256-266: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClean up the new English strings before they fan out to translations.
There are several obvious typos in the new msgids, including
powerfull,follwoing, andvehile_net. Since this is the POT source, every locale will inherit these on the next merge.Also applies to: 595-682
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/pot/basic/graphs.pot` around lines 256 - 266, The new English msgids in the POT source contain obvious typos that will be propagated to every locale on the next merge. Review the added strings in the graphs-related entries and correct the misspellings such as powerfull, follwoing, and vehile_net at the source before regeneration so the translated catalogs inherit clean text. Also check the related msgids in the broader affected range to ensure all new strings are normalized consistently.locale/pot/basic/vehicle.pot-245-246 (1)
245-246: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the exercise title typo.
Create a penalazied viewshould be corrected before this heading becomes the stable msgid for translators and docs readers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/pot/basic/vehicle.pot` around lines 245 - 246, The exercise title msgid contains a typo and needs to be corrected before it is used by translators and docs readers. Update the string in the vehicle POT entry so the heading text in the msgid uses the correct spelling, and keep the surrounding generated reference/comment structure intact.locale/pot/basic/pedestrian.pot-47-49 (1)
47-49: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the new msgid typos.
This template introduces a few avoidable typos in user-facing strings:
oms2pgrouting,Don'f, andsanme.Also applies to: 75-77, 386-387
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/pot/basic/pedestrian.pot` around lines 47 - 49, Fix the typoed user-facing msgids in the pedestrian template by correcting the string containing oms2pgrouting, Don'f, and sanme in the affected msgid entries. Update the relevant msgid text in pedestrian.pot consistently across all matching occurrences (including the ones around the referenced sections) so the translation template uses the intended spellings and readable wording.locale/es/LC_MESSAGES/basic/sql_function.po-357-359 (1)
357-359: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't switch this exercise title to English in the Spanish catalog.
The section title and its later inline reference now display in English inside the Spanish docs. Translate the title again and point the later reference at the same Spanish label.
Also applies to: 477-481
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/es/LC_MESSAGES/basic/sql_function.po` around lines 357 - 359, The Spanish catalog entry for the exercise title was changed to English and the later inline reference now points to that English text. Restore the translated Spanish string in the msgstr for the exercise title in basic/sql_function.po, and update the matching later reference so it uses the same Spanish label consistently; check the related entries around the other referenced range as well to keep both occurrences aligned.locale/pot/general-intro/overview.pot-63-73 (1)
63-73: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRepair the feature-list text before regenerating the template.
The A* entry has mismatched backticks, and
commponentis misspelled. That will render poorly in the generated docs and propagate bad strings to translators.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/pot/general-intro/overview.pot` around lines 63 - 73, The feature-list strings in the overview template need cleanup before regeneration: the A* entry has mismatched reStructuredText backticks and “commponent” is misspelled. Fix the msgid text in the overview pot entries for the A* Search Algorithm and Graph component functions so the markup renders correctly and the source strings are translator-ready.locale/pot/un_sdg/data.pot-23-25 (1)
23-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the duplicated words in the intro paragraph.
use ``osm2pgrouting`` to get that the datareads as a copy error in the extracted text. Please fix the source paragraph and regenerate the POT.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/pot/un_sdg/data.pot` around lines 23 - 25, The extracted msgid in the un_sdg intro paragraph contains a duplicated/garbled phrase around the osm2pgrouting sentence, so fix the source text in the corresponding documentation paragraph (the one that feeds the data entry) to remove the copy error and make the sentence read naturally, then regenerate the POT so the updated msgid is exported consistently.locale/es/LC_MESSAGES/basic/graphs.po-374-376 (1)
374-376: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the Spanish exercise label.
Both
msgstrvalues now use the English title, so the Spanish build will render that heading/reference in English. Keep the section title translated and reuse the same Spanish text in the later cross-reference.Also applies to: 585-587
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/es/LC_MESSAGES/basic/graphs.po` around lines 374 - 376, The Spanish translation for the exercise heading has been replaced with the English text, so restore the translated label in the affected msgstr entries and keep the same Spanish wording for the later cross-reference as well. Update the matching strings in the graphs.po entry for the exercise title so the section heading and its reference both use the Spanish translation consistently.locale/pot/general-intro/introduction.pot-89-91 (1)
89-91: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the acknowledgments heading.
Aknowledmentsis user-visible text in the extracted template. Please fix the source heading toAcknowledgments/Acknowledgementsand regenerate this catalog.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/pot/general-intro/introduction.pot` around lines 89 - 91, The extracted template contains a misspelled user-visible heading in the introduction content. Update the source heading that feeds this msgid from “Aknowledments” to the correct “Acknowledgments” or “Acknowledgements”, then regenerate the pot catalog so the msgid reflects the corrected text; use the introduction heading/source that produces the msgid in the introduction docs.locale/pot/general-intro/overview.pot-47-49 (1)
47-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the duplicated phrase in this overview sentence.
This extracted string now says
PostreSQLand repeatscan be modified, so the published overview will ship with visible copy errors. Fix the source paragraph and regenerate the POT.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/pot/general-intro/overview.pot` around lines 47 - 49, The overview sentence in the extracted msgid has two copy errors: “PostreSQL” is misspelled and “can be modified” is duplicated. Fix the source paragraph that feeds the overview string so it uses the correct database name and only one “can be modified” phrase, then regenerate the POT so the updated msgid is reflected in the translation template.locale/es/LC_MESSAGES/basic/vehicle.po-209-211 (1)
209-211: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the Spanish exercise titles and references localized.
These
msgstrvalues were changed to English labels, so the Spanish chapter will mix English headings/reference text into an otherwise translated page. Restore the Spanish wording consistently for both titles and their comparison links.Also applies to: 279-281, 429-431, 552-554
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locale/es/LC_MESSAGES/basic/vehicle.po` around lines 209 - 211, The Spanish translation entries for the vehicle exercise headings and their comparison-link text were switched back to English, so restore the localized Spanish wording in the affected msgstr values. Update the matching entries in vehicle.po for the exercise title strings and the corresponding comparison references so the Spanish chapter stays fully translated and consistent across all listed occurrences.docs/basic/pedestrian.rst-40-40 (1)
40-40: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix typo: "Don'f" → "Don't"
Line 40: "Don'f forget to connect to the database" contains a typo.
📝 Proposed fix
- Don'f forget to connect to the database, if not connected: + Don't forget to connect to the database, if not connected:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/basic/pedestrian.rst` at line 40, Fix the typo in the pedestrian documentation sentence so it reads “Don’t forget to connect to the database, if not connected:”. Update the text in the relevant docs/basic/pedestrian.rst content and keep the rest of the sentence unchanged.docs/basic/graphs.rst-648-648 (1)
648-648: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix typo: "follwoing" → "following"
Line 648: "all follwoing
id" contains a typo.📝 Proposed fix
- * Get a traveling cost matrix in seconds from the all follwoing ``id`` to all + * Get a traveling cost matrix in seconds from the all following ``id`` to all🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/basic/graphs.rst` at line 648, There is a typo in the graphs documentation text: the phrase in the travel cost matrix description uses “follwoing” instead of “following”. Update the affected sentence in the graphs docs so the wording reads naturally and correctly, keeping the rest of the description unchanged.docs/basic/graphs.rst-729-729 (1)
729-729: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix typo: "vehile_net" → "vehicle_net"
Line 729: "
vehile_net" is a typo in the exercise description.📝 Proposed fix
- * The results when using ``vehile_net`` is the example. + * The results when using ``vehicle_net`` is the example.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/basic/graphs.rst` at line 729, There is a typo in the exercise text: the identifier shown as vehile_net should be vehicle_net. Update the wording in the graphs documentation where the example result mentions this symbol so the exercise description matches the intended name exactly.docs/basic/pedestrian.rst-364-364 (1)
364-364: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix typo: "sanme" → "same"
Line 364: "The sanme results" contains a typo.
📝 Proposed fix
-The sanme results can be obtained from the ``pedestrian_many_to_many`` view: +The same results can be obtained from the ``pedestrian_many_to_many`` view:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/basic/pedestrian.rst` at line 364, Fix the typo in the pedestrian documentation sentence so it reads “same results” instead of “sanme results.” Update the wording in the pedestrian_many_to_many view description to correct the misspelling while keeping the rest of the sentence unchanged.docs/basic/pedestrian.rst-28-28 (1)
28-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix typo: "oms2pgrouting" → "osm2pgrouting"
Line 28: "
oms2pgrouting" is misspelled.📝 Proposed fix
-``oms2pgrouting`` +``osm2pgrouting``🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/basic/pedestrian.rst` at line 28, The documentation contains a typo in the pedestrian guide: the package name is written incorrectly as oms2pgrouting. Update the referenced term in the pedestrian documentation to the correct osm2pgrouting spelling, keeping the same inline code formatting so readers can find the right tool name.docs/basic/graphs.rst-224-224 (1)
224-224: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix typo: "powerfull" → "powerful"
Line 224: "QGIS is a powerfull tool" contains a typo.
📝 Proposed fix
-QGIS is a powerfull tool +QGIS is a powerful tool🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/basic/graphs.rst` at line 224, Correct the typo in the QGIS documentation sentence so “powerfull” becomes “powerful”. Update the text in the relevant docs/basic/graphs.rst content where the sentence “QGIS is a powerfull tool” appears, keeping the rest of the wording unchanged.docs/basic/vehicle.rst-179-179 (1)
179-179: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix typo: "penalazied" → "penalized"
Line 179: "Create a penalazied view" contains a typo.
📝 Proposed fix
-Add a penalty column on the :code:`configuration` table with default value of 1. -Create a view that uses the value of the column to modify the costs. +Add a penalty column on the :code:`configuration` table with default value of 1. +Create a view that uses the value of the column to modify the costs.(The typo is in the heading at line 179: "penalazied")
-Exercise 4: Create a penalazied view +Exercise 4: Create a penalized view🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/basic/vehicle.rst` at line 179, Fix the typo in the Exercise 4 heading: the text in the vehicle documentation currently says “penalazied” and should be changed to “penalized”. Update the heading text in the docs section for Exercise 4 so the title reads correctly and remains consistent with the surrounding documentation style.docs/scripts/basic/vehicles/vehicles.sql-95-99 (1)
95-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis check is too weak to prove the routes are the same.
Filtering both views to
edge = -1only compares the terminal row, so route regressions can slip through while this file still says the results match. Compare the full projected paths instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/scripts/basic/vehicles/vehicles.sql` around lines 95 - 99, The current validation in vehicles.sql only compares the terminal row via edge = -1, which is too weak to prove the routes match. Update the check around vehicle_route_coming and vehicle_penalized_route to compare the full projected path/result set rather than just the last row, so regressions anywhere in the route are detected.docs/scripts/basic/vehicles/images.sql-25-26 (1)
25-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe comment and the SQL disagree here.
Line 25 says residential roads are penalized by 3x, but Line 26 applies
penalty=5. Please update one of them so the workshop text matches the actual query.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/scripts/basic/vehicles/images.sql` around lines 25 - 26, The workshop text and the SQL in the configuration update are inconsistent: the comment on the `UPDATE configuration` block says residential roads are penalized by 3x, but the query in `images.sql` sets `penalty=5` for `tag_value` residential. Update either the comment or the `penalty` value in that block so the wording and the actual `UPDATE configuration` behavior match, using the `UPDATE configuration` statement as the reference point.docs/scripts/basic/pedestrian/pedestrian.sql-37-52 (1)
37-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the terminal
edge = -1row inpedestrian_one_to_one.This inner join drops the last
pgr_dijkstrarow, soone_to_one_view.txtloses the path terminator while the other pedestrian views keep it.Suggested fix
SELECT dijkstra.*, geom FROM dijkstra -JOIN walk_net ON(edge = id) +LEFT JOIN walk_net ON(edge = id) ORDER BY seq;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/scripts/basic/pedestrian/pedestrian.sql` around lines 37 - 52, The `pedestrian_one_to_one` view is dropping the terminal `edge = -1` row because `dijkstra` is joined to `walk_net` with an inner join. Update the `pedestrian_one_to_one` SELECT so it preserves all `pgr_dijkstra` rows, including the path terminator, by changing the join behavior in the `dijkstra`/`walk_net` part of the view definition (while keeping the existing columns and ordering). Ensure the `pedestrian_one_to_one` result matches the other pedestrian views by retaining the final row from `pgr_dijkstra`.
🧹 Nitpick comments (8)
docs/general-intro/introduction.rst (2)
67-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix inconsistent name list formatting.
The "Past and present tutors and developers" section mixes formatting styles: "Daniel Kastl," and "Iosefa Persival," appear as separate comma-terminated lines while the remaining names flow together. This renders as uneven paragraphs. Use a consistent format—either a single comma-separated paragraph or a proper bulleted list.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/general-intro/introduction.rst` around lines 67 - 70, The “Past and present tutors and developers” block has inconsistent name-list formatting, with Daniel Kastl and Iosefa Persival split into separate comma-terminated lines while the rest flows differently. Update the list in the introduction.rst content to use one consistent style throughout—either keep all names in a single comma-separated paragraph or convert the whole section into a uniform bulleted list.
82-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapitalize "erosion" to match other supporter names.
"erosion" is lowercase while "Georepublic" and "Paragon Corporation" are title case. Change to "Erosion" for consistency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/general-intro/introduction.rst` at line 82, The supporter name in the introduction text is inconsistent with the other title-cased names. Update the occurrence of erosion in the introduction content to Erosion so it matches Georepublic and Paragon Corporation; this is a simple text change in the introduction.rst section.docs/un_sdg/sdg3-health.rst (3)
193-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix typo: "buidling" → "building".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/un_sdg/sdg3-health.rst` at line 193, Fix the typo in the SDG3 health documentation text by changing the misspelled word in the sentence about columns that are not related to a building concept; update the wording in the affected documentation block so it uses the correct spelling and keeps the rest of the sentence unchanged.
546-546: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate
gidreference toidto match schema migration.The text describes
closest_vertexas returning "the gid of the closest vertex," but the workshop SQL and data have migrated fromgid/the_geomtoid/geom. Update this description to avoid confusing attendees when the actual column isid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/un_sdg/sdg3-health.rst` at line 546, The `closest_vertex` description still refers to the old `gid` column and should be updated to match the migrated schema. In the `sdg3-health` documentation, change the wording around the `closest_vertex` output to reference `id` instead of `gid`, keeping the rest of the sentence consistent with the current `id`/`geom` schema used in the workshop materials.
185-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix typo: "LINGSTING" → "LINESTRING".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/un_sdg/sdg3-health.rst` at line 185, Fix the typo in the documentation sentence describing buildings_ways: replace “LINGSTING” with “LINESTRING”. Update the wording in the sdg3-health.rst text so the geometry type is spelled correctly and matches the expected GIS term.docs/interactions/chapter-9.rst (1)
136-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate stale workshop location reference.
Line 136 still references
pgrouting-Bucharest-Examplein the save project name, but this PR is migrating the workshop to Hiroshima. This stale reference will confuse users following the Hiroshima instructions.-#. Navigate to: :menuselection:`User --> Desktop --> workshop` and save ``pgrouting-Bucharest-Example`` +#. Navigate to: :menuselection:`User --> Desktop --> workshop` and save ``pgrouting-Hiroshima-Example``🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/interactions/chapter-9.rst` around lines 136 - 137, The workshop instructions still reference the old Bucharest project name, so update the save-project step in the chapter-9 workshop text to use the Hiroshima naming instead. Locate the navigation/save instruction in the chapter 9 document and replace the stale pgrouting-Bucharest-Example reference with the correct Hiroshima-specific project name so the docs stay consistent with the migrated workshop..github/workflows/ubuntu.yml (1)
48-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the osm2pgrouting bootstrap into one shared script.
This block is duplicated here and in
.github/workflows/website.ymlLines 64-72. The next dependency/CMake tweak now has to land twice, which is easy to drift and leaves one pipeline green while the other breaks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ubuntu.yml around lines 48 - 57, The osm2pgrouting build bootstrap is duplicated in the Ubuntu workflow and the website workflow, so future CMake or dependency changes can drift between pipelines. Extract the shared setup steps from the Build osm2pgrouting from source block into a reusable script or composite action, then have both workflows call that shared entry point so updates only need to be made once.docs/basic/vehicle.rst (1)
221-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typo in SQL anchor name: "comming" → "coming"
The
:end-before:anchorsame_result_as_comming.txtcontains a typo. While this is an internal SQL file marker, it should be corrected for consistency and to avoid confusion. Please verify the corresponding anchor indocs/scripts/basic/vehicles/vehicles.sqlis also updated.📝 Proposed fix
- :end-before: same_result_as_comming.txt + :end-before: same_result_as_coming.txtAnd in the SQL file, update:
--- same_result_as_comming.txt +++ same_result_as_coming.txt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/basic/vehicle.rst` at line 221, The SQL anchor name referenced by the docs has a typo: the `:end-before:` marker uses `same_result_as_comming.txt` instead of the intended spelling. Update the anchor in the documentation and make sure the matching marker in `vehicles.sql` is renamed consistently to `same_result_as_coming.txt` so both references stay in sync.
| #--------------------------------------------- | ||
| set(SPHINX_MINIMUM_VERSION "4.0.0") | ||
| set(Osm2pgrouting_FIND_VERSION "2.3") | ||
| set(Osm2pgrouting_FIND_VERSION "3.0.0") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Re-enable the osm2pgrouting version guard.
set(Osm2pgrouting_FIND_VERSION "3.0.0") does not currently enforce anything: the supplied cmake/FindOsm2pgrouting.cmake still has the version comparison wrapped in if (false). That lets an older osm2pgrouting configure successfully and only surface the incompatibility later. Please restore the check in the find-module, or compare Osm2pgrouting_VERSION explicitly after discovery.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CMakeLists.txt` at line 101, Re-enable the osm2pgrouting version guard by
updating the FindOsm2pgrouting flow so the version requirement is actually
enforced. The current set(Osm2pgrouting_FIND_VERSION "3.0.0") is ineffective
because the check in cmake/FindOsm2pgrouting.cmake is disabled; restore the
version comparison there or add an explicit post-discovery comparison against
Osm2pgrouting_VERSION so older installs fail during configure. Use the
FindOsm2pgrouting.cmake logic and the
Osm2pgrouting_VERSION/Osm2pgrouting_FIND_VERSION symbols to locate the fix.
| SELECT * | ||
| FROM vehicle_net | ||
| WHERE vehicle_net.the_geom && ST_MakeEnvelope(39.27, -6.79, 39.30, -6.83); | ||
| WHERE vehicle_net.geom && ST_MakeEnvelope(39.27, -6.79, 39.30, -6.83); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace the leftover pre-Hiroshima coordinates.
On Line 281 and Lines 463/475/487, the hard-coded bbox/point still uses the old 39.x/-6.x location instead of Hiroshima (~132.x/34.x). That makes little_net empty for the Hiroshima dataset and causes the nearest-vertex checks to snap to unrelated features, so these exercises will no longer reproduce the documented results.
Also applies to: 463-487
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/interactions/code/functions.sql` at line 281, The hard-coded spatial
coordinates in the SQL examples are still using the old 39.x/-6.x location
instead of the Hiroshima dataset, which breaks the documented outputs. Update
the bbox and point literals in the affected queries inside functions.sql,
including the statements around the little_net setup and the nearest-vertex
checks, so they use the Hiroshima ~132.x/34.x coordinates consistently and
continue to reference the same query examples.
| SELECT * | ||
| FROM vehicle_net | ||
| WHERE vehicle_net.the_geom && ST_MakeEnvelope(39.27, -6.79, 39.30, -6.83); | ||
| WHERE vehicle_net.geom && ST_MakeEnvelope(39.27, -6.79, 39.30, -6.83); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace the leftover pre-Hiroshima coordinates.
On Line 281 and Lines 463/475/487, the hard-coded bbox/point still uses the old 39.x/-6.x location instead of Hiroshima (~132.x/34.x). That makes little_net empty for the Hiroshima dataset and causes the nearest-vertex checks to snap to unrelated features, so these exercises will no longer reproduce the documented results.
Also applies to: 463-487
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/interactions/code/QGISfunctions.sql` at line 281, The hard-coded spatial
literals in the QGISfunctions.sql exercises still use the old 39.x/-6.x
bbox/point values, so update the affected queries in the little_net setup and
nearest-vertex checks to use the Hiroshima 132.x/34.x coordinates instead.
Locate the literals in the queries around the vehicle_net/little_net sections
and the vertex snapping checks, and replace every leftover pre-Hiroshima
bbox/point so the documented results are reproducible.
|
|
||
|
|
||
| \o set_components1.txt | ||
| ALTER TABLE ways ADD COLUMN component BIGINT; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make this schema change rerunnable.
Line 43 will abort the second execution once ways.component already exists, which is awkward for rebuilds and workshop reruns.
Suggested fix
-ALTER TABLE ways ADD COLUMN component BIGINT;
+ALTER TABLE ways ADD COLUMN IF NOT EXISTS component BIGINT;📝 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.
| ALTER TABLE ways ADD COLUMN component BIGINT; | |
| ALTER TABLE ways ADD COLUMN IF NOT EXISTS component BIGINT; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/scripts/basic/graphs/graphs.sql` at line 43, Make the schema change in
the ways table rerunnable by guarding the ADD COLUMN operation so repeated
executions do not fail once ways.component already exists. Update the ALTER
TABLE statement in the graphs.sql migration to use an existence check or
equivalent idempotent pattern, keeping the change localized around the ways
component column addition.
| add_custom_target(basic_pedestrian_scripts) | ||
|
|
||
| set(GENERATED_FILES | ||
| combinations.txt | ||
| get_id.txt | ||
| many_to_many.txt | ||
| many_to_one.txt | ||
| note_1.txt | ||
| one_to_many.txt | ||
| one_to_one.txt | ||
| dijkstracost.txt | ||
| ) | ||
| configure_file(pedestrian.sql pedestrian.sql) | ||
|
|
||
| #---------------------------------------------- | ||
| # Generating the results files | ||
| #--------------------------------------------- | ||
|
|
||
| add_custom_command( | ||
| COMMAND psql -d city_routing -f pedestrian.sql | ||
| COMMAND psql -d city_routing -f images.sql | ||
| OUTPUT ${GENERATED_FILES} | ||
| TARGET basic_pedestrian_scripts | ||
| PRE_BUILD | ||
| BYPRODUCTS | ||
| combinations.txt | ||
| get_id.txt | ||
| many_to_many.txt | ||
| many_to_one.txt | ||
| note_1.txt | ||
| one_to_many.txt | ||
| one_to_one.txt | ||
| one_to_one_view.txt | ||
| dijkstracost.txt | ||
| stars.txt | ||
|
|
||
| DEPENDS pedestrian.sql images.sql | ||
| COMMAND psql -d city_routing -f pedestrian.sql | ||
| COMMENT "running chapter pedestrian scripts" | ||
| ) | ||
|
|
||
| add_custom_target(basic_pedestrian_scripts | ||
| DEPENDS ${GENERATED_FILES} | ||
| ) | ||
| DEPENDS pedestrian.sql) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
sed -n '1,120p' docs/scripts/basic/pedestrian/CMakeLists.txtRepository: pgRouting/workshop
Length of output: 821
🏁 Script executed:
sed -n '1,120p' docs/scripts/basic/pedestrian/CMakeLists.txtRepository: pgRouting/workshop
Length of output: 821
🌐 Web query:
CMake add_custom_command TARGET PRE_BUILD behavior non-Visual Studio generators custom target portability
💡 Result:
The PRE_BUILD option in add_custom_command has specific, non-portable behavior that varies depending on the CMake generator being used [1][2]. For Visual Studio generators, PRE_BUILD commands are executed before any other rules are executed within that target [1][3]. However, for all other generators (such as Makefile generators or Ninja), PRE_BUILD is not supported in the same way and behaves identically to PRE_LINK [1][2]. This means that on non-Visual Studio generators, the command will run after the target's sources have been compiled but before the final link step, which is often too late if the command is intended to generate files required for compilation (e.g., headers) [4][3]. Due to this lack of portability, it is strongly recommended to avoid using PRE_BUILD unless you are certain the project will only be built with Visual Studio generators [1][5]. To achieve pre-build behavior in a portable, generator-agnostic manner, the standard approach is to use add_custom_target combined with add_dependencies [6][7]. 1. Define a custom target to execute your command using add_custom_target. 2. Ensure the command generates the necessary files (e.g., using add_custom_command with an OUTPUT). 3. Make your main target (the library or executable) depend on the custom target using add_dependencies [7]. This pattern forces CMake to build the custom target before the main target, ensuring that your command executes at the desired time regardless of the underlying build system [7][8].
Citations:
- 1: https://cmake.org/cmake/help/latest/command/add_custom_command.html
- 2: https://cmake.org/cmake/help/v3.27/command/add_custom_command.html
- 3: https://cmake.org/cmake/help/v3.23/command/add_custom_command.html
- 4: https://discourse.cmake.org/t/pre-ninja-bash-command/1418
- 5: https://cmake.org/cmake/help/v4.3/command/add_custom_command.html
- 6: https://stackoverflow.com/questions/75878531/how-can-i-ensure-a-custom-cmake-command-is-running-before-any-dependencies-are-b
- 7: https://stackoverflow.com/questions/15972898/cmake-how-to-run-a-add-custom-command-before-everything-else
- 8: https://stackoverflow.com/questions/37862072/cmake-run-custom-command-before-build
🌐 Web query:
CMake add_custom_command TARGET PRE_BUILD behavior non-Visual Studio generators custom target portability
💡 Result:
In CMake, the PRE_BUILD option for add_custom_command has specialized behavior exclusively for Visual Studio generators [1][2]. When using Visual Studio generators, a PRE_BUILD command executes before any other rules are processed for that target [1][3]. For all other generators (such as Makefile generators or Ninja), PRE_BUILD is treated identically to PRE_LINK [1][2]. Because of this non-portable behavior, it is explicitly recommended to avoid using PRE_BUILD unless you are certain that a Visual Studio generator is in use [1][3]. If you require pre-build or pre-link actions that must work across different generators, you should consider using PRE_LINK instead, or using a separate custom target (created via add_custom_target) with explicit dependencies (via add_dependencies) to control the build order [4]. Key points regarding portability: - Visual Studio Generators: PRE_BUILD runs before any other rules for the target [1][5]. - Other Generators (Make/Ninja): PRE_BUILD behaves exactly like PRE_LINK [1][2][5]. - PRE_LINK: Executes after source files are compiled but before the target is linked or the archiver/librarian runs [5]. This is generally the more portable choice if you need an action to occur before the final binary creation. - Policy Considerations: As of CMake 3.31 (policy CMP0175), the TARGET form of add_custom_command requires exactly one of PRE_BUILD, PRE_LINK, or POST_BUILD to be specified [6]. Projects should ensure their CMake code adheres to this to avoid warnings or future compatibility errors [6].
Citations:
- 1: https://cmake.org/cmake/help/latest/command/add_custom_command.html
- 2: https://cmake.org/cmake/help/v3.27/command/add_custom_command.html
- 3: https://cmake.org/cmake/help/latest/command/add_custom_command.html?highlight=add_custom_command
- 4: https://stackoverflow.com/questions/25029895/cmake-custom-build-step-pre-link-pre-build-do-not-work
- 5: https://cedanet.com.au/cmake/build-events.php
- 6: https://cmake.org/cmake/help/latest/policy/CMP0175.html
🌐 Web query:
CMake add_custom_command TARGET custom target PRE_BUILD add_custom_target behavior Ninja Makefiles
💡 Result:
In CMake, the behavior of add_custom_command with the PRE_BUILD option varies significantly depending on the generator used [1][2]. On Visual Studio generators, PRE_BUILD executes the specified command before any other rules within that target are executed [1][2]. However, on all other generators—including Ninja and Makefile generators—PRE_BUILD is not supported in the same way and is instead treated as PRE_LINK [1][3]. This means the command will run after the target's sources have been compiled but before the final link step, which is often too late if you intended to generate source files or headers required for compilation [3][4]. Because of this inconsistency, it is recommended to avoid using PRE_BUILD unless you are specifically targeting Visual Studio [1][5]. If you need a command to run before the compilation of a target (for example, to generate header files or source code), you should use the other signature of add_custom_command that defines an OUTPUT [4]. By specifying the file(s) to be generated as the OUTPUT and listing them as sources for your target (e.g., using target_sources), CMake will automatically ensure the command runs as needed to generate those files before they are required by the build [4]. Alternatively, for tasks that must run every time a target is built or to create complex dependency chains, you can use add_custom_target combined with add_dependencies to ensure the target is executed before the main build target [6][7].
Citations:
- 1: https://cmake.org/cmake/help/latest/command/add_custom_command.html
- 2: https://cmake.org/cmake/help/v3.23/command/add_custom_command.html
- 3: https://cmake.org/cmake/help/latest/command/add_custom_command.html?highlight=add_custom_command
- 4: https://stackoverflow.com/questions/76184365/can-i-use-add-custom-command-pre-build-on-a-target-to-generate-sources
- 5: https://cmake.org/cmake/help/v4.3/command/add_custom_command.html
- 6: https://stackoverflow.com/questions/26216570/cmake-add-custom-command-with-pre-build-does-not-work
- 7: https://discourse.cmake.org/t/pre-ninja-bash-command/1418
🌐 Web query:
CMake add_custom_command TARGET custom target PRE_BUILD add_custom_target behavior Ninja Makefiles
💡 Result:
In CMake, the add_custom_command option PRE_BUILD has highly specific behavior that varies depending on the generator used [1][2]. For Visual Studio generators, PRE_BUILD executes the specified command before any other rules within the target are executed [1][3]. For all other generators, including Ninja and Makefiles, PRE_BUILD is not supported in the same way and is treated as PRE_LINK [1][4][2]. PRE_LINK commands are executed after the target's dependencies are built but before the target itself (the linker) is invoked [2]. Because of this inconsistency, it is standard practice to avoid using PRE_BUILD unless you are specifically targeting Visual Studio [1][3]. If you require a command to run unconditionally before a target is built across all generators (such as Ninja or Makefiles), the recommended approach is to use add_custom_target combined with add_dependencies [4]. 1. Use add_custom_target to define your command as a named target [5][6]. 2. Use add_dependencies to make your main target depend on the custom target [4][5]. This ensures that the custom target is evaluated and executed every time the main target is built [5][7]. Note that add_custom_target is always considered out of date [5][7], so it will execute its command whenever it is triggered. If you need to perform an operation only when specific input files change, use add_custom_command(OUTPUT ...) instead, which provides efficient, dependency-based rebuilding [7][8].
Citations:
- 1: https://cmake.org/cmake/help/latest/command/add_custom_command.html
- 2: https://cmake.org/cmake/help/v3.0/command/add_custom_command.html?highlight=pre_build
- 3: https://cmake.org/cmake/help/v4.3/command/add_custom_command.html
- 4: https://stackoverflow.com/questions/26216570/cmake-add-custom-command-with-pre-build-does-not-work
- 5: https://stackoverflow.com/questions/11971917/why-does-cmake-make-a-distinction-between-a-target-and-a-command
- 6: https://cmake.org/cmake/help/book/mastering-cmake/chapter/Custom%20Commands.html
- 7: https://fekir.info/post/cmake-custom-targets-and-commands/
- 8: https://offlinemark.com/cmake-custom-build-stage/
🌐 Web query:
CMake add_custom_command TARGET PRE_BUILD custom target supported custom target PRE_LINK PRE_BUILD behavior
💡 Result:
The add_custom_command(TARGET...) command allows attaching build events to targets like executables or libraries [1][2]. These events are categorized by timing, and their behavior varies based on the underlying generator [1][2]. Build Event Timing and Behavior: PRE_BUILD: On Visual Studio generators, this command executes before any other rules within the target [1][2]. On all other generators, it behaves identically to PRE_LINK [1][2]. Because of this inconsistency, it is recommended to avoid PRE_BUILD unless you are specifically targeting Visual Studio environments [1][3]. PRE_LINK: This command runs after the target's source files have been compiled, but before the linker, librarian, or archiver tool is invoked [1][2]. This option is not supported for targets created by add_custom_target [1][2]. POST_BUILD: This command runs after all other rules within the target have completed execution [1][2]. It is commonly used for post-processing binaries, such as generating hashes or performing code signing [4]. Custom Target Support: The TARGET form of add_custom_command is primarily designed for built-in target types like executables and libraries [1]. It is explicitly noted that PRE_LINK is not defined for targets created by add_custom_target [1][2]. If you need to perform actions associated with a custom target, it is typically more appropriate to use the COMMAND arguments directly within the add_custom_target definition, or to use add_dependencies to orchestrate the build order [5][6]. Policy Note: Projects should always specify one of the three event keywords (PRE_BUILD, PRE_LINK, or POST_BUILD) when using the TARGET form of add_custom_command, in accordance with policy CMP0175 [3][4].
Citations:
- 1: https://cmake.org/cmake/help/v4.3/command/add_custom_command.html
- 2: https://cmake.org/cmake/help/v3.23/command/add_custom_command.html
- 3: https://cmake.org/cmake/help/latest/command/add_custom_command.html?highlight=add_custom_command
- 4: https://github.com/Kitware/CMake/blob/master/Help/command/add_custom_command.rst
- 5: https://cmake.org/cmake/help/latest/command/add_custom_target.html
- 6: https://cmake.org/cmake/help/book/mastering-cmake/chapter/Custom%20Commands.html
🌐 Web query:
CMake add_custom_command TARGET PRE_BUILD custom target supported custom target PRE_LINK PRE_BUILD behavior
💡 Result:
In CMake, the add_custom_command(TARGET...) command is used to attach build events to an existing target (such as an executable or library) defined in the same directory [1][2][3]. The PRE_BUILD, PRE_LINK, and POST_BUILD keywords define when these commands execute [1][4]. Behavior of PRE_BUILD The PRE_BUILD option is specifically designed for Visual Studio generators [1][4]. On these generators, the command runs before any other rules are executed for that target [1][5]. For all other generators (such as Makefile or Ninja), PRE_BUILD is not supported as a distinct phase and behaves identically to PRE_LINK [1][4]. Due to this inconsistency, it is strongly recommended to avoid PRE_BUILD unless you are exclusively targeting Visual Studio [1][6]. Behavior of PRE_LINK The PRE_LINK command runs after the target's sources have been compiled but before the actual linking step (or before the librarian/archiver runs for static libraries) [1][2][4]. This option is not supported for targets created by add_custom_target [1][4]. Summary of Key Constraints and Policies 1. Generator Specifics: Because PRE_BUILD behaves differently across generators, code should be designed to handle these variations or avoid PRE_BUILD entirely [1][6]. 2. Target Definition: The target must be defined in the same directory as the add_custom_command call [1][4]. 3. Policy CMP0175: Starting with CMake 3.31, the TARGET form of add_custom_command strictly requires exactly one of PRE_BUILD, PRE_LINK, or POST_BUILD to be specified [7]. Previous versions may have assumed a default or allowed missing/multiple keywords, but modern CMake enforces these as errors [7]. 4. Custom Targets: While add_custom_command can attach events to targets, if you are using add_custom_target (which creates a custom target that executes its own commands), you should generally rely on the commands defined within that target rather than attempting to use PRE_LINK or PRE_BUILD on it [1][3].
Citations:
- 1: https://cmake.org/cmake/help/git-master/command/add_custom_command.html
- 2: https://cmake.org/cmake/help/latest/command/add_custom_command.html
- 3: https://cmake.org/cmake/help/book/mastering-cmake/chapter/Custom%20Commands.html
- 4: https://cmake.org/cmake/help/v3.22/command/add_custom_command.html
- 5: https://stackoverflow.com/questions/26216570/cmake-add-custom-command-with-pre-build-does-not-work
- 6: https://cmake.org/cmake/help/v4.3/command/add_custom_command.html
- 7: https://cmake.org/cmake/help/latest/policy/CMP0175.html
🌐 Web query:
site:cmake.org add_custom_command TARGET PRE_BUILD add_custom_target custom target not defined PRE_LINK
💡 Result:
The error message "custom target not defined" or similar issues when using add_custom_command with PRE_BUILD or PRE_LINK often arise from two specific limitations in CMake: 1. Target Scope Limitations: The TARGET specified in add_custom_command must be defined in the same directory (CMakeLists.txt file) as the add_custom_command call [1][2]. If the target is defined in a different directory, CMake will not be able to locate it, resulting in an error [1][3][4]. 2. add_custom_target Compatibility: The PRE_LINK and PRE_BUILD build events are specifically designed for "real" targets like libraries and executables [1][5]. They are not supported for targets created by add_custom_target() [1][2]. Consequently, you cannot attach a PRE_LINK or PRE_BUILD command to a custom target; if you attempt to use one, CMake may reject it, especially under stricter policies like CMP0175 [6]. Recommended Workarounds: - If you need to run a command before a target (including a custom target) that is defined in another directory, use add_dependencies() [7][8]. - Create a new add_custom_target() that executes your desired command, and then use add_dependencies(your_main_target your_custom_target) to ensure the command runs as part of the build process [4][7][8]. - Ensure you are using the TARGET form of add_custom_command correctly, which now requires exactly one of PRE_BUILD, PRE_LINK, or POST_BUILD [6]. Note that PRE_BUILD is primarily effective only with Visual Studio generators; for other generators, it behaves like PRE_LINK [1][2].
Citations:
- 1: https://cmake.org/cmake/help/latest/command/add_custom_command.html
- 2: https://cmake.org/cmake/help/v3.23/command/add_custom_command.html
- 3: https://cmake.org/pipermail/cmake/2016-January/062533.html
- 4: https://discourse.cmake.org/t/run-custom-command-before-some-project-build-in-superbuild/4152
- 5: https://cmake.org/cmake/help/v4.3/command/add_custom_command.html
- 6: https://cmake.org/cmake/help/latest/policy/CMP0175.html
- 7: https://cmake.org/cmake/help/latest/command/add_custom_target.html
- 8: https://cmake.org/cmake/help/latest/command/add%5Fcustom%5Ftarget.html
🌐 Web query:
site:cmake.org add_custom_command TARGET PRE_BUILD add_custom_target custom target not defined PRE_LINK
💡 Result:
The error regarding PRE_LINK or PRE_BUILD not being defined occurs because these options are specifically designed for targets that undergo a compilation and linking process, such as libraries or executables [1][2]. Targets created using the add_custom_target command do not have a linking step, and therefore, they do not support the PRE_LINK or PRE_BUILD event hooks [1][3]. To clarify the usage of these options: PRE_LINK and PRE_BUILD are only defined for targets created by commands like add_executable or add_library [1][3]. Because add_custom_target creates a target with no output file and no standard build rules (like compilation or linking), CMake does not support attaching pre-build or pre-link events to them [1][4][3]. If you need to execute commands before a custom target runs, you should use add_dependencies to ensure other targets or commands complete before the custom target executes, or integrate the command directly into the add_custom_target definition [4][5]. Note that PRE_BUILD has unique, generator-specific behavior: it is only natively supported by Visual Studio generators [1][2]. On other generators (like Makefiles or Ninja), it is treated as a synonym for PRE_LINK [1][6]. Due to this inconsistency, it is generally recommended to avoid PRE_BUILD unless you are specifically targeting Visual Studio environments [1][2].
Citations:
- 1: https://cmake.org/cmake/help/latest/command/add_custom_command.html?highlight=add_custom_command
- 2: https://cmake.org/cmake/help/v3.27/command/add_custom_command.html
- 3: https://cmake.org/Bug/view.php?id=15059
- 4: https://cmake.org/cmake/help/latest/command/add_custom_target.html
- 5: https://cmake.org/cmake/help/latest/guide/tutorial/Custom%20Commands%20and%20Generated%20Files.html
- 6: https://cmake.org/cmake/help/v3.23/command/add_custom_command.html
Replace the PRE_BUILD hook on basic_pedestrian_scripts
basic_pedestrian_scripts is a custom target, and the TARGET form of add_custom_command() does not support PRE_BUILD on custom targets. Move the psql command directly into add_custom_target(...) or switch back to an OUTPUT-based custom command with explicit dependencies.
docs/scripts/basic/pedestrian/CMakeLists.txt:4-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/scripts/basic/pedestrian/CMakeLists.txt` around lines 4 - 29, The
current add_custom_command on basic_pedestrian_scripts uses TARGET with
PRE_BUILD, which is not supported for a custom target. Update the CMake logic
around add_custom_target(basic_pedestrian_scripts) and add_custom_command so the
psql invocation is attached in a supported way: either move the command into
add_custom_target(...) as the target’s command, or convert it to an OUTPUT-based
custom command with explicit dependencies on pedestrian.sql and the generated
txt files.
| CREATE OR REPLACE VIEW vehicle_penalty_routes AS | ||
| WITH dijkstra AS ( | ||
| SELECT * FROM pgr_dijkstra( | ||
| ' SELECT gid AS id, source, target, cost_s * penalty AS cost, reverse_cost_s * penalty AS reverse_cost | ||
| ' SELECT id, source, target, cost_s * penalty AS cost, reverse_cost_s * penalty AS reverse_cost | ||
| FROM ways JOIN configuration | ||
| USING (tag_id) ', | ||
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_5@], | ||
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_5@]) | ||
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_3@], | ||
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_3@]) | ||
| ) | ||
| SELECT seq, | ||
| CASE WHEN start_vid = @ID_1@ THEN '@PLACE_1@' WHEN start_vid = @ID_2@ THEN '@PLACE_2@' | ||
| WHEN start_vid = @ID_3@ THEN '@PLACE_3@' WHEN start_vid = @ID_4@ THEN '@PLACE_4@' | ||
| WHEN start_vid = @ID_5@ THEN '@PLACE_5@' END | ||
| WHEN start_vid = @ID_3@ THEN '@PLACE_5@' END | ||
| ||','|| | ||
| CASE WHEN end_vid = @ID_1@ THEN '@PLACE_1@' WHEN end_vid = @ID_2@ THEN '@PLACE_2@' | ||
| WHEN end_vid = @ID_3@ THEN '@PLACE_3@' WHEN end_vid = @ID_4@ THEN '@PLACE_4@' | ||
| WHEN end_vid = @ID_5@ THEN '@PLACE_5@' END | ||
| WHEN end_vid = @ID_3@ THEN '@PLACE_5@' END | ||
| AS name, | ||
| start_vid, end_vid, the_geom AS geom | ||
| FROM dijkstra JOIN ways ON(edge = gid); | ||
| start_vid, end_vid, geom AS geom | ||
| FROM dijkstra JOIN ways ON(edge = id); | ||
|
|
||
| CREATE OR REPLACE VIEW vehicle_no_penalty_routes AS | ||
| WITH dijkstra AS ( | ||
| SELECT * FROM pgr_dijkstra( | ||
| ' SELECT gid AS id, source, target, cost_s AS cost, reverse_cost_s AS reverse_cost FROM ways ', | ||
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_5@], | ||
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_5@]) | ||
| ' SELECT id, source, target, cost_s AS cost, reverse_cost_s AS reverse_cost FROM ways ', | ||
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_3@], | ||
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_3@]) | ||
| ) | ||
| SELECT seq, | ||
| CASE WHEN start_vid = @ID_1@ THEN '@PLACE_1@' WHEN start_vid = @ID_2@ THEN '@PLACE_2@' | ||
| WHEN start_vid = @ID_3@ THEN '@PLACE_3@' WHEN start_vid = @ID_4@ THEN '@PLACE_4@' | ||
| WHEN start_vid = @ID_5@ THEN '@PLACE_5@' END | ||
| WHEN start_vid = @ID_3@ THEN '@PLACE_5@' END | ||
| ||','|| | ||
| CASE WHEN end_vid = @ID_1@ THEN '@PLACE_1@' WHEN end_vid = @ID_2@ THEN '@PLACE_2@' | ||
| WHEN end_vid = @ID_3@ THEN '@PLACE_3@' WHEN end_vid = @ID_4@ THEN '@PLACE_4@' | ||
| WHEN end_vid = @ID_5@ THEN '@PLACE_5@' END | ||
| WHEN end_vid = @ID_3@ THEN '@PLACE_5@' END | ||
| AS name, | ||
| start_vid, end_vid, the_geom AS geom | ||
| FROM dijkstra JOIN ways ON(edge = gid); | ||
| start_vid, end_vid, geom AS geom | ||
| FROM dijkstra JOIN ways ON(edge = id); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore @ID_5@ in these route sets and label branches.
Both views now repeat @ID_3@ in the source/target arrays and in the CASE mapping, so the fifth route pair is replaced by a second @ID_3@ run and @PLACE_5@ is never matched.
Suggested fix
-ARRAY[`@ID_1`@,`@ID_2`@,`@ID_3`@,`@ID_4`@, `@ID_3`@],
-ARRAY[`@ID_1`@,`@ID_2`@,`@ID_3`@,`@ID_4`@, `@ID_3`@])
+ARRAY[`@ID_1`@,`@ID_2`@,`@ID_3`@,`@ID_4`@, `@ID_5`@],
+ARRAY[`@ID_1`@,`@ID_2`@,`@ID_3`@,`@ID_4`@, `@ID_5`@])
...
- WHEN start_vid = `@ID_3`@ THEN '`@PLACE_5`@' END
+ WHEN start_vid = `@ID_5`@ THEN '`@PLACE_5`@' END
...
- WHEN end_vid = `@ID_3`@ THEN '`@PLACE_5`@' END
+ WHEN end_vid = `@ID_5`@ THEN '`@PLACE_5`@' END📝 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 VIEW vehicle_penalty_routes AS | |
| WITH dijkstra AS ( | |
| SELECT * FROM pgr_dijkstra( | |
| ' SELECT gid AS id, source, target, cost_s * penalty AS cost, reverse_cost_s * penalty AS reverse_cost | |
| ' SELECT id, source, target, cost_s * penalty AS cost, reverse_cost_s * penalty AS reverse_cost | |
| FROM ways JOIN configuration | |
| USING (tag_id) ', | |
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_5@], | |
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_5@]) | |
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_3@], | |
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_3@]) | |
| ) | |
| SELECT seq, | |
| CASE WHEN start_vid = @ID_1@ THEN '@PLACE_1@' WHEN start_vid = @ID_2@ THEN '@PLACE_2@' | |
| WHEN start_vid = @ID_3@ THEN '@PLACE_3@' WHEN start_vid = @ID_4@ THEN '@PLACE_4@' | |
| WHEN start_vid = @ID_5@ THEN '@PLACE_5@' END | |
| WHEN start_vid = @ID_3@ THEN '@PLACE_5@' END | |
| ||','|| | |
| CASE WHEN end_vid = @ID_1@ THEN '@PLACE_1@' WHEN end_vid = @ID_2@ THEN '@PLACE_2@' | |
| WHEN end_vid = @ID_3@ THEN '@PLACE_3@' WHEN end_vid = @ID_4@ THEN '@PLACE_4@' | |
| WHEN end_vid = @ID_5@ THEN '@PLACE_5@' END | |
| WHEN end_vid = @ID_3@ THEN '@PLACE_5@' END | |
| AS name, | |
| start_vid, end_vid, the_geom AS geom | |
| FROM dijkstra JOIN ways ON(edge = gid); | |
| start_vid, end_vid, geom AS geom | |
| FROM dijkstra JOIN ways ON(edge = id); | |
| CREATE OR REPLACE VIEW vehicle_no_penalty_routes AS | |
| WITH dijkstra AS ( | |
| SELECT * FROM pgr_dijkstra( | |
| ' SELECT gid AS id, source, target, cost_s AS cost, reverse_cost_s AS reverse_cost FROM ways ', | |
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_5@], | |
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_5@]) | |
| ' SELECT id, source, target, cost_s AS cost, reverse_cost_s AS reverse_cost FROM ways ', | |
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_3@], | |
| ARRAY[@ID_1@,@ID_2@,@ID_3@,@ID_4@, @ID_3@]) | |
| ) | |
| SELECT seq, | |
| CASE WHEN start_vid = @ID_1@ THEN '@PLACE_1@' WHEN start_vid = @ID_2@ THEN '@PLACE_2@' | |
| WHEN start_vid = @ID_3@ THEN '@PLACE_3@' WHEN start_vid = @ID_4@ THEN '@PLACE_4@' | |
| WHEN start_vid = @ID_5@ THEN '@PLACE_5@' END | |
| WHEN start_vid = @ID_3@ THEN '@PLACE_5@' END | |
| ||','|| | |
| CASE WHEN end_vid = @ID_1@ THEN '@PLACE_1@' WHEN end_vid = @ID_2@ THEN '@PLACE_2@' | |
| WHEN end_vid = @ID_3@ THEN '@PLACE_3@' WHEN end_vid = @ID_4@ THEN '@PLACE_4@' | |
| WHEN end_vid = @ID_5@ THEN '@PLACE_5@' END | |
| WHEN end_vid = @ID_3@ THEN '@PLACE_5@' END | |
| AS name, | |
| start_vid, end_vid, the_geom AS geom | |
| FROM dijkstra JOIN ways ON(edge = gid); | |
| start_vid, end_vid, geom AS geom | |
| FROM dijkstra JOIN ways ON(edge = id); | |
| CREATE OR REPLACE VIEW vehicle_penalty_routes AS | |
| WITH dijkstra AS ( | |
| SELECT * FROM pgr_dijkstra( | |
| ' SELECT id, source, target, cost_s * penalty AS cost, reverse_cost_s * penalty AS reverse_cost | |
| FROM ways JOIN configuration | |
| USING (tag_id) ', | |
| ARRAY[`@ID_1`@,`@ID_2`@,`@ID_3`@,`@ID_4`@, `@ID_5`@], | |
| ARRAY[`@ID_1`@,`@ID_2`@,`@ID_3`@,`@ID_4`@, `@ID_5`@]) | |
| ) | |
| SELECT seq, | |
| CASE WHEN start_vid = `@ID_1`@ THEN '`@PLACE_1`@' WHEN start_vid = `@ID_2`@ THEN '`@PLACE_2`@' | |
| WHEN start_vid = `@ID_3`@ THEN '`@PLACE_3`@' WHEN start_vid = `@ID_4`@ THEN '`@PLACE_4`@' | |
| WHEN start_vid = `@ID_5`@ THEN '`@PLACE_5`@' END | |
| ||','|| | |
| CASE WHEN end_vid = `@ID_1`@ THEN '`@PLACE_1`@' WHEN end_vid = `@ID_2`@ THEN '`@PLACE_2`@' | |
| WHEN end_vid = `@ID_3`@ THEN '`@PLACE_3`@' WHEN end_vid = `@ID_4`@ THEN '`@PLACE_4`@' | |
| WHEN end_vid = `@ID_5`@ THEN '`@PLACE_5`@' END | |
| AS name, | |
| start_vid, end_vid, geom AS geom | |
| FROM dijkstra JOIN ways ON(edge = id); | |
| CREATE OR REPLACE VIEW vehicle_no_penalty_routes AS | |
| WITH dijkstra AS ( | |
| SELECT * FROM pgr_dijkstra( | |
| ' SELECT id, source, target, cost_s AS cost, reverse_cost_s AS reverse_cost FROM ways ', | |
| ARRAY[`@ID_1`@,`@ID_2`@,`@ID_3`@,`@ID_4`@, `@ID_5`@], | |
| ARRAY[`@ID_1`@,`@ID_2`@,`@ID_3`@,`@ID_4`@, `@ID_5`@]) | |
| ) | |
| SELECT seq, | |
| CASE WHEN start_vid = `@ID_1`@ THEN '`@PLACE_1`@' WHEN start_vid = `@ID_2`@ THEN '`@PLACE_2`@' | |
| WHEN start_vid = `@ID_3`@ THEN '`@PLACE_3`@' WHEN start_vid = `@ID_4`@ THEN '`@PLACE_4`@' | |
| WHEN start_vid = `@ID_5`@ THEN '`@PLACE_5`@' END | |
| ||','|| | |
| CASE WHEN end_vid = `@ID_1`@ THEN '`@PLACE_1`@' WHEN end_vid = `@ID_2`@ THEN '`@PLACE_2`@' | |
| WHEN end_vid = `@ID_3`@ THEN '`@PLACE_3`@' WHEN end_vid = `@ID_4`@ THEN '`@PLACE_4`@' | |
| WHEN end_vid = `@ID_5`@ THEN '`@PLACE_5`@' END | |
| AS name, | |
| start_vid, end_vid, geom AS geom | |
| FROM dijkstra JOIN ways ON(edge = id); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/scripts/basic/vehicles/images.sql` around lines 49 - 87, The route views
are missing the fifth identifier and currently duplicate `@ID_3`@ in both the
pgr_dijkstra input arrays and the CASE label mapping, so the fifth route is
never selected correctly. Update vehicle_penalty_routes and
vehicle_no_penalty_routes to use `@ID_5`@ in the arrays and add the corresponding
WHEN branches in the name CASE expressions so `@PLACE_5`@ maps to the correct
route pair.
| FROM bangladesh JOIN waterways.waterways_ways w | ||
| ON (ST_Intersects(w.geom, get_city_buffer(5))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the uncorrelated bangladesh join.
The ON clause never references the row from bangladesh, so every matching waterway is duplicated once per row in that table. Either drop bangladesh from the FROM clause here or join the specific city row and use its city_buffer directly.
Suggested fix
-FROM bangladesh JOIN waterways.waterways_ways w
-ON (ST_Intersects(w.geom, get_city_buffer(5)));
+FROM waterways.waterways_ways w
+WHERE ST_Intersects(w.geom, get_city_buffer(5));📝 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.
| FROM bangladesh JOIN waterways.waterways_ways w | |
| ON (ST_Intersects(w.geom, get_city_buffer(5))); | |
| FROM waterways.waterways_ways w | |
| WHERE ST_Intersects(w.geom, get_city_buffer(5)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/scripts/un_sdg/sdg11/all_exercises_sdg11.sql` around lines 93 - 94, The
query currently joins bangladesh without using it in the ON clause, so the
waterway rows are multiplied unnecessarily. Update the SELECT that uses
waterways.waterways_ways w and get_city_buffer(5) to either remove bangladesh
from the FROM clause entirely or change it to join the specific city row and
reference that row’s city_buffer in the ST_Intersects call.
| CREATE OR REPLACE FUNCTION closest_edge(geom GEOMETRY) | ||
| RETURNS BIGINT AS | ||
| $BODY$ | ||
| SELECT gid FROM roads_ways ORDER BY geom <-> the_geom LIMIT 1; | ||
| SELECT id FROM roads_ways ORDER BY geom <-> geom LIMIT 1; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
sed -n '180,230p' docs/scripts/un_sdg/sdg3/all_exercises_sdg3.sqlRepository: pgRouting/workshop
Length of output: 1282
🏁 Script executed:
grep -n "closest_edge\|roads_ways\|<->" -n docs/scripts/un_sdg/sdg3/all_exercises_sdg3.sqlRepository: pgRouting/workshop
Length of output: 1529
🏁 Script executed:
sed -n '140,155p' docs/scripts/un_sdg/sdg3/all_exercises_sdg3.sqlRepository: pgRouting/workshop
Length of output: 668
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('docs/scripts/un_sdg/sdg3/all_exercises_sdg3.sql')
lines = p.read_text().splitlines()
for i in range(145, 152):
print(f"{i+1}:{lines[i]}")
PYRepository: pgRouting/workshop
Length of output: 348
Use the input geometry in closest_edge
docs/scripts/un_sdg/sdg3/all_exercises_sdg3.sql:204-207 still orders by geom <-> geom, so the query compares the road geometry to itself. Match closest_vertex and reference the function argument explicitly ($1 or a renamed parameter) while qualifying roads_ways.geom.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/scripts/un_sdg/sdg3/all_exercises_sdg3.sql` around lines 204 - 207, The
closest_edge function is comparing roads_ways.geom to itself instead of the
input geometry, so update the SELECT in closest_edge to use the function
argument explicitly and qualify the table column. Mirror the pattern used by
closest_vertex, and reference the parameter as $1 or rename it clearly so the
ORDER BY compares roads_ways.geom against the input geometry.
| msgid "Requirements for routing from A to B" | ||
| msgstr "Requisitos para el ruteo de A a B" | ||
| msgstr "Requirements for routing from A to B" | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't replace the Spanish headings and refs with English text.
These msgstrs now render English titles/links in the Spanish docs (Requirements for routing from A to B, Exercise 3: Nearest Vertex, etc.). If this was done to keep the references resolving, please fix the source withPoints.rst with explicit labels and keep the translated strings Spanish here.
Also applies to: 351-352, 527-528, 568-568, 619-620
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@locale/es/LC_MESSAGES/basic/withPoints.po` around lines 50 - 52, The Spanish
PO entries are incorrectly using English text for headings/refs, so update the
source in withPoints.rst to add explicit labels for the referenced sections and
then restore the localized Spanish msgstr values here. Focus on the affected
translated strings like “Requirements for routing from A to B” and the other
listed headings so the Spanish docs keep Spanish titles while links still
resolve correctly.
| #: ../../build/docs/basic/pedestrian.rst:284 | ||
| msgid "First pedestrian goes from \"|place_1|\" to \"|place_4|\"" | ||
| msgstr "" | ||
|
|
||
| #: ../../build/docs/basic/pedestrian.rst:255 | ||
| msgid "Second pedestrian goes from \"|place_2|\" to \"|place_5|\"" | ||
| #: ../../build/docs/basic/pedestrian.rst:285 | ||
| msgid "Second pedestrian goes from \"|place_3|\" to \"|place_5|\"" | ||
| msgstr "" | ||
|
|
||
| #: ../../build/docs/basic/pedestrian.rst:264 | ||
| msgid "First pedestrian departs from |id_1| and the destination is |id_4| (line **6**)." | ||
| #: ../../build/docs/basic/pedestrian.rst:294 | ||
| msgid "First pedestrian departs from |id_1| and the destination is |id_3|." | ||
| msgstr "" | ||
|
|
||
| #: ../../build/docs/basic/pedestrian.rst:265 | ||
| msgid "Second pedestrian departs from |id_2| and the destination is |id_5| (line **7**)." | ||
| #: ../../build/docs/basic/pedestrian.rst:295 | ||
| msgid "Second pedestrian departs from |id_2| and the destination is |id_5|." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exercise 5 is internally inconsistent.
The problem statement says the routes are |place_1| -> |place_4| and |place_3| -> |place_5|, but the solution switches them to |id_1| -> |id_3| and |id_2| -> |id_5|. That gives readers the wrong endpoints for the exercise.
Suggested fix
-msgid "First pedestrian departs from |id_1| and the destination is |id_3|."
+msgid "First pedestrian departs from |id_1| and the destination is |id_4|."
-msgid "Second pedestrian departs from |id_2| and the destination is |id_5|."
+msgid "Second pedestrian departs from |id_3| and the destination is |id_5|."📝 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.
| #: ../../build/docs/basic/pedestrian.rst:284 | |
| msgid "First pedestrian goes from \"|place_1|\" to \"|place_4|\"" | |
| msgstr "" | |
| #: ../../build/docs/basic/pedestrian.rst:255 | |
| msgid "Second pedestrian goes from \"|place_2|\" to \"|place_5|\"" | |
| #: ../../build/docs/basic/pedestrian.rst:285 | |
| msgid "Second pedestrian goes from \"|place_3|\" to \"|place_5|\"" | |
| msgstr "" | |
| #: ../../build/docs/basic/pedestrian.rst:264 | |
| msgid "First pedestrian departs from |id_1| and the destination is |id_4| (line **6**)." | |
| #: ../../build/docs/basic/pedestrian.rst:294 | |
| msgid "First pedestrian departs from |id_1| and the destination is |id_3|." | |
| msgstr "" | |
| #: ../../build/docs/basic/pedestrian.rst:265 | |
| msgid "Second pedestrian departs from |id_2| and the destination is |id_5| (line **7**)." | |
| #: ../../build/docs/basic/pedestrian.rst:295 | |
| msgid "Second pedestrian departs from |id_2| and the destination is |id_5|." | |
| #: ../../build/docs/basic/pedestrian.rst:284 | |
| msgid "First pedestrian goes from \"|place_1|\" to \"|place_4|\"" | |
| msgstr "" | |
| #: ../../build/docs/basic/pedestrian.rst:285 | |
| msgid "Second pedestrian goes from \"|place_3|\" to \"|place_5|\"" | |
| msgstr "" | |
| #: ../../build/docs/basic/pedestrian.rst:294 | |
| msgid "First pedestrian departs from |id_1| and the destination is |id_4|." | |
| msgstr "" | |
| #: ../../build/docs/basic/pedestrian.rst:295 | |
| msgid "Second pedestrian departs from |id_3| and the destination is |id_5|." |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@locale/pot/basic/pedestrian.pot` around lines 330 - 343, The Exercise 5
strings in pedestrian.pot are inconsistent between the problem statement and the
solution; keep the endpoints aligned. Update the solution text under the
matching pedestrian entries so it reflects the same routes as the statement (the
first and second pedestrian pairs from the surrounding msgids), and verify the
msgids/msgstrs in the pedestrian section stay synchronized.
|
The rabbit is anoying at this moment, I havent reached further chapters |
2 participants
Changes proposed in this pull request:
Summary by CodeRabbit
New Features
Bug Fixes
Chores