Channel Rules -> Rules + some inconsistency fixes/cleanup #239
Channel Rules -> Rules + some inconsistency fixes/cleanup #239umair-ably merged 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Walkthrough: This PR renames channel rules to rules in all user-facing text, standardizes ID label formatting, removes the expose-time-serial flag and exposeTimeSerial field, and deletes legacy channel-rule and apps channel-rules alias commands with their tests. Changes: Deleted 10 deprecated alias command files in apps/channel-rules and channel-rule namespaces. Updated apps/rules commands (create, update, delete, list, index) to use rule terminology and remove expose-time-serial. Removed exposeTimeSerial from channel-rule-display.ts. Removed 8 test files for deleted aliases. Updated rules tests to match new output. Updated e2e tests for rules terminology. Regenerated README and updated Project-Structure.md. Breaking changes: ably channel-rule and ably apps channel-rules are deleted; expose-time-serial flag removed from rules create and update. No new dependencies. Test coverage complete. |
![claude-code-ably-assistant[bot]](https://avatars.githubusercontent.com/in/2109912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, focused PR. The rename is thorough and the alias cleanup is correct. One real inconsistency to flag.
Finding: list.ts line 103 — namespace.id not wrapped in formatResource()
The change introduces:
this.log(`${formatHeading("ID")} ${namespace.id}`);But it should be:
this.log(`${formatHeading("ID")} ${formatResource(namespace.id)}`);The ID renders as plain text in list output, while create/delete/update all display it in cyan via formatResource(). The PR description says this standardizes ID formatting, but the list output ends up as the only command where the ID is not cyan. Project convention: always use formatResource() for resource names.
The formatHeading label is fine for a list record header — just the value needs formatResource().
Everything else looks correct: rename is exhaustive across commands/tests/e2e/docs/README; exposeTimeSerial cleanly removed from flag, JSON output interface, human-readable display helper, and request payload; service layer Namespace interface correctly retains exposeTimeSerial (API still has the field, CLI just stops surfacing it); all 10 alias files and their tests properly deleted; test strings updated to match new output; no this.error() calls, no broken JSON guards, no missing test coverage.
WalkthroughThis PR renames "channel rules" to "rules" throughout all user-facing CLI text to align with Ably's documentation and dashboard terminology. It also standardizes ID formatting in list output, removes the deprecated Changes
Review Notes
|
umair-ably
force-pushed
the
rulesFixes
branch
3 times, most recently
from
c68627d to
ca4f895
Compare
There was a problem hiding this comment.
REVIEW SUMMARY
Overall this is a clean, well-scoped rename. One formatting bug slipped in.
FINDING: src/commands/apps/rules/list.ts Line 103
Formatting inconsistency. The PR description says it is standardising ID formatting to match create/update/delete, but the list command ends up using different formatters.
What is there now:
this.log(${formatHeading("ID")} ${namespace.id});
What create/update/delete all use:
this.log(${formatLabel("ID")} ${formatResource(namespace.id)});
The difference matters visually:
- formatHeading = bold, no colon appended
- formatLabel = dim text with colon, the standard field-label style used everywhere else
- namespace.id without formatResource = plain text, not cyan
So after "ably apps rules create chat" the user sees a dim ID: label + cyan ID. Then in "ably apps rules list" they see a bold ID (no colon) + plain-text ID for the same field. That is the opposite of what the PR intended.
Fix: use formatLabel("ID") and formatResource(namespace.id) to match create/update/delete.
BREAKING CHANGES (intentional and documented)
The removal of --expose-time-serial from create and update, and the deletion of the channel-rule and apps channel-rules alias commands, are all breaking changes. These appear intentional per the PR description. Worth confirming they are covered in a changelog or release note if that is part of this project's process.
EVERYTHING ELSE LOOKS GOOD
- exposeTimeSerial is cleanly removed from output without breaking the underlying type (field still in Namespace, API still returns it, CLI just stops exposing it)
- Error handling correctly uses this.fail() throughout; component strings are camelCase (ruleDelete, ruleUpdate, ruleList)
- All 5 required describe blocks present in modified test files
- No auth flags in unit tests, no --duration in unit tests
- Tests correctly updated to match new output strings
|
Fixed the formatting issue mentioned by Claude |
|
Previously we had alias for |
yep, deleted. We're pre-GA so best to use the opportunity to clean it up now |
…ides exposeTimeserial in Rules - Deletes deprecated Rules alias
…reate, add null-handling fallbacks and modified timestamp to create/update, align list ID heading with CLI convention
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Thanks for addressing feedback, LGTM
2 participants
Summary
Ably's documentation and dashboard refer to these as "rules", not "channel rules". This aligns the CLI with that terminology.
ID: {id}label instead ofChannel Rule ID: {id}heading)exposeTimeSerialfrom CLI output (JSON, human-readable) and the--expose-time-serialflag, as it's no longer encouragedchannel-ruleandapps channel-rulesalias commands and their testsTest plan
ably apps rules listshowsID:label (notChannel Rule ID:)ably apps rules list --jsondoes not includeexposeTimeSerialably apps rules create --helpdoes not show--expose-time-serialflagably channel-ruleno longer exists as a command