Skip to content

Followup changes for https://github.com/hi-godot/godot-ai/pull/446#453

Merged
dsarno merged 2 commits into
hi-godot:mainfrom
a-johnston:add_telemetry_option_followup
May 17, 2026
Merged

Followup changes for https://github.com/hi-godot/godot-ai/pull/446#453
dsarno merged 2 commits into
hi-godot:mainfrom
a-johnston:add_telemetry_option_followup

Conversation

@a-johnston

Copy link
Copy Markdown
Contributor

This is a grab bag of changes following up on the copilot review of #446. Many of the changes are test additions, comment changes, or md changes. Common setting constants used between modules and helpers are moved to plugin/addons/godot_ai/utils/settings.gd.

@codecov

codecov Bot commented May 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Followup to PR #446. Centralizes shared EditorSettings key constants (SETTING_HTTP_PORT, SETTING_EXCLUDED_DOMAINS, SETTING_TELEMETRY_ENABLED) and small helpers (truthy, env_truthy, telemetry_enabled) into a new plugin/addons/godot_ai/utils/settings.gd (McpSettings), and adds tests/comments for the telemetry toggle plumbing.

Changes:

  • Introduce McpSettings utility holding setting keys and env/truthy helpers, and route client_configurator.gd, mcp_dock.gd, server_lifecycle.gd, telemetry.gd, plugin.gd, and test code through it.
  • Add GDScript tests for the telemetry toggle UI, _inject_telemetry_env lifecycle behavior, and the EditorSetting fallback in Telemetry; tighten the Python test_cleanup_logs_warning_on_oserror to assert the warning message.
  • Update self-update smoke harness + its test to patch port/telemetry constants from their new home (utils/settings.gd); refresh docs/comments to describe the env-var + UI opt-out priority.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
plugin/addons/godot_ai/utils/settings.gd(.uid) New McpSettings class with shared keys + truthy/telemetry helpers.
plugin/addons/godot_ai/client_configurator.gd Move SETTING_HTTP_PORT / SETTING_EXCLUDED_DOMAINS to McpSettings.
plugin/addons/godot_ai/mcp_dock.gd Use McpSettings keys/helpers; rename local var; wrap min_size in Vector2i; tweak tooltip and reload typing.
plugin/addons/godot_ai/plugin.gd Drop _env_truthy, delegate to McpSettings.truthy; inject telemetry env around start_dev_server spawn.
plugin/addons/godot_ai/telemetry.gd Delegate disable resolution to McpSettings.telemetry_enabled(); update docs.
plugin/addons/godot_ai/utils/server_lifecycle.gd Simplify _inject_telemetry_env via McpSettings.telemetry_enabled().
src/godot_ai/telemetry.py Doc/comment tweaks reflecting cleanup-on-disable behavior.
docs/TELEMETRY.md Document UI + env-var opt-out paths together.
script/local-self-update-smoke Patch settings keys in new utils/settings.gd path.
tests/unit/test_self_update_smoke_harness.py Assert keys in utils/settings.gd.
tests/unit/test_telemetry.py Assert warning is logged on cleanup OSError.
test_project/tests/test_telemetry_setting.gd Migrate to McpSettings.SETTING_TELEMETRY_ENABLED.
test_project/tests/test_telemetry_toggle.gd(.uid) New tests for the dock's telemetry CheckButton disabled state.
test_project/tests/test_plugin_telemetry.gd Add tests for EditorSetting-driven enable/disable in Telemetry.
test_project/tests/test_server_lifecycle.gd Add tests for _inject_telemetry_env across env/setting combos.
test_project/tests/test_clients.gd Update setting references to McpSettings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test_project/tests/test_telemetry_setting.gd
Comment thread test_project/tests/test_telemetry_toggle.gd
Comment thread test_project/tests/test_server_lifecycle.gd
Comment thread test_project/tests/test_plugin_telemetry.gd
Comment thread plugin/addons/godot_ai/telemetry.gd Outdated
- mcp_dock.gd: clarify the Vector2i wrap with a comment explaining that
  `Vector2i * float` returns `Vector2`, and use `Vector2(560, 460)` as
  the literal so the cast reads as intentional instead of doubled.
- telemetry.gd: normalize tab indentation and "MCP dock" casing in the
  opt-out priority docstring.
- test_telemetry_setting.gd: remove the contradictory "may or may not
  remove the key depending on Godot version" caveat; passing null to
  EditorSettings.set_setting has been the documented unset path for
  many years.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dsarno dsarno merged commit f45282c into hi-godot:main May 17, 2026
11 checks passed
@dsarno

dsarno commented May 17, 2026

Copy link
Copy Markdown
Contributor

Pushed 1fb2b88 before merging to address review nits 1–3:

  • mcp_dock.gd — wrote the Vector2i(...) cast as Vector2i(Vector2(560, 460) * scale) and added a comment explaining that Vector2i * float → Vector2, so the cast reads as intentional instead of doubled.
  • telemetry.gd — normalized the tab indentation and "MCP dock" casing in the opt-out priority docstring.
  • test_telemetry_setting.gd — removed the contradictory "may or may not remove the key depending on Godot version" caveat (per @a-johnston's note that set_setting(..., null) has been the documented unset path for many years).

All 11 CI checks green; squash-merged as f45282c.

@dsarno

dsarno commented May 17, 2026

Copy link
Copy Markdown
Contributor

Thanks again @a-johnston !

@a-johnston

Copy link
Copy Markdown
Contributor Author

I had to be afk for a few hours but this was still in progress. I'll make a followup to the followup pr.

@dsarno

dsarno commented May 17, 2026

Copy link
Copy Markdown
Contributor

Ah roger thought it was done. I had a cluster of PRs to push out today and I try to get them all in one release to isolate issues picked by telemetry.

@a-johnston a-johnston deleted the add_telemetry_option_followup branch May 17, 2026 22:08
@a-johnston

Copy link
Copy Markdown
Contributor Author

No problem, the changes again were all test surface and comments. I should've noted that I actually have a new test file to add which was missed when I first committed and pushed this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants