Skip to content

chore(comments): remove section markers#1546

Merged
leshy merged 1 commit intodevfrom
paul/fix/remove-section-markers
Mar 14, 2026
Merged

chore(comments): remove section markers#1546
leshy merged 1 commit intodevfrom
paul/fix/remove-section-markers

Conversation

@paul-nechifor
Copy link
Contributor

Problem

Section markers are added by Claude. We don't need them. If a file is too complicated, it shouldn't use sections, it should be split into smaller files.

Closes DIM-XXX

Solution

  • Removed section markers.
  • Reformatted good comments.

Breaking Changes

None.

How to Test

None.

Contributor License Agreement

  • I have read and approved the CLA.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 14, 2026

Greptile Summary

This PR removes Claude-generated section-marker comments (e.g. # ============= Section =============, # --- Section ---, # region/# endregion) from 68 source files and adds a new pytest test (dimos/test_no_sections.py) that enforces the policy going forward by scanning the entire repo on each CI run.

Key changes:

  • All section-style separator/header comments removed across Python, YAML, Dockerfile, and markdown files.
  • Some "good" comments (e.g. # Robot Management) are kept as plain # Comment lines — these do not trigger any test pattern.
  • dimos/test_no_sections.py is a new linting test using os.walk + regex to detect violations; it correctly handles whitelisting and extension filtering.
  • Two minor issues found in the new test: the ".egg-info" entry in IGNORED_DIRS won't match real egg-info directory names (e.g. dimos.egg-info), and the _is_ignored_dir(dirpath) guard on line 96 is dead code because the dirnames[:] = in-place pruning already prevents os.walk from ever entering ignored directories.

Confidence Score: 5/5

  • Safe to merge — all changes are comment/formatting removals with no logic impact; the new enforcement test is correct and will pass.
  • All 68 modified files contain only comment deletions or reformatting. No executable logic was changed anywhere. The new test correctly identifies section markers and will reliably prevent regressions. The two issues found (.egg-info pattern mismatch and redundant _is_ignored_dir check) are harmless in practice and do not affect test correctness or CI stability.
  • Only dimos/test_no_sections.py warrants a second look for the minor logic issues noted in comments.

Important Files Changed

Filename Overview
dimos/test_no_sections.py New enforcement test that scans the repo for section-style comment markers; contains a redundant _is_ignored_dir guard and a .egg-info pattern that won't match real egg-info directory names.
dimos/manipulation/planning/monitor/world_monitor.py Section markers converted to plain # Section Name comments; no logic changes, looks correct.
dimos/manipulation/planning/world/drake_world.py Section markers converted to plain comments and one # --- Meshcat proxies --- header removed entirely; no logic changes.
dimos/protocol/pubsub/impl/shmpubsub.py File-level header block and inline section markers removed/simplified; removal of the header leaves a double blank line before from __future__, minor style issue only.
dimos/skills/skills.py Both # region / # endregion markers and # ==== ... ==== inline markers removed; no logic changes.
docker/navigation/.env.hardware Section separator blocks removed from the env template; all variable definitions are preserved intact.
docker/navigation/Dockerfile Stage header comments and separator blocks removed; all build instructions are unchanged.
docs/capabilities/manipulation/adding_a_custom_arm.md Section marker blocks removed from code examples in the guide; leaves occasional double blank lines in the markdown but content is unaffected.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[test_no_section_markers] --> B[find_section_markers]
    B --> C[os.walk REPO_ROOT]
    C --> D[Prune ignored dirs in-place]
    D --> E[Visit files in dir]
    E --> F{_should_scan?}
    F -->|py / yml / yaml / Dockerfile| G[Read file line by line]
    F -->|Other extensions| C
    G --> H{Line matches pattern?}
    H -->|SEPARATOR_LINE| I{Whitelisted?}
    H -->|INLINE_SECTION| I
    H -->|REGION_MARKER| I
    H -->|No match| G
    I -->|Yes| G
    I -->|No| J[Append to violations]
    J --> G
    G --> C
    C --> K{violations empty?}
    K -->|Yes| L[Test passes]
    K -->|No| M[AssertionError with report]
Loading

Last reviewed commit: eb643da

Comment on lines +43 to +55
IGNORED_DIRS = {
".venv",
"venv",
"__pycache__",
"node_modules",
".git",
"dist",
"build",
".egg-info",
".tox",
# third-party vendored code
"gtsam",
}
Copy link
Contributor

Choose a reason for hiding this comment

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

.egg-info pattern won't match real egg-info directory names

".egg-info" is listed as a literal directory name to ignore, but actual egg-info directories are named like dimos.egg-info or mypackage.egg-info — never the bare string ".egg-info". Neither the dirnames[:] = [d for d in dirnames if d not in IGNORED_DIRS] pruning nor _is_ignored_dir will ever match a real egg-info directory.

In practice this is harmless because egg-info directories don't contain .py / .yml / .yaml files that would be scanned, but the entry is misleading and the protection it implies doesn't work. Consider replacing it with an endswith check or using a glob-style pattern:

# In _should_scan or an _is_ignored_dir replacement:
if any(part.endswith(".egg-info") for part in dirpath.split(os.sep)):
    continue

Or simply remove the ".egg-info" entry from IGNORED_DIRS and add an endswith guard at the top of the scanning loop.

Comment on lines +92 to +97
for dirpath, dirnames, filenames in os.walk(REPO_ROOT):
# Prune ignored directories in-place
dirnames[:] = [d for d in dirnames if d not in IGNORED_DIRS]

if _is_ignored_dir(dirpath):
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

_is_ignored_dir check is unreachable dead code

The in-place dirnames[:] = [d for d in dirnames if d not in IGNORED_DIRS] mutation on line 94 tells os.walk never to descend into ignored directories. As a result, after the very first iteration (where dirpath == REPO_ROOT), every subsequent dirpath is guaranteed to be a non-ignored directory. The _is_ignored_dir(dirpath) guard on line 96 will therefore never return True in practice — it only triggers if REPO_ROOT itself contains an ignored directory component in its absolute path, which is essentially impossible.

Consider removing the dead check to avoid misleading future readers:

Suggested change
for dirpath, dirnames, filenames in os.walk(REPO_ROOT):
# Prune ignored directories in-place
dirnames[:] = [d for d in dirnames if d not in IGNORED_DIRS]
if _is_ignored_dir(dirpath):
continue
for dirpath, dirnames, filenames in os.walk(REPO_ROOT):
# Prune ignored directories in-place
dirnames[:] = [d for d in dirnames if d not in IGNORED_DIRS]
rel_dir = os.path.relpath(dirpath, REPO_ROOT)

@leshy leshy merged commit 482bfcf into dev Mar 14, 2026
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants