Add a lint lesson for Naming/PredicateMethod #994

New issue

Closed

opened 2026-10-11 22:47:38 +00:00 by jared · 1 comment

jared commented

2026-10-11 22:47:38 +00:00

Owner

Problem

plugins/os-sdlc/lint-lessons/ has no lesson for Naming/PredicateMethod. In n8n-api, 13 resource methods always returned true. The cop flagged them, and a pipeline programmer turned the cop off for the whole directory (commit c51ebb3, jaredswanson/n8n-api#2). The cop message suggests a ? rename, which was also wrong: these methods are commands, not questions. After #989 every lint path shows lessons, so a lesson for this cop reaches the next fixer. This is the pain point that started #989.

	Code	Result
Offense	`def delete(id) … true end`	The cop says a method that returns only a boolean must be a predicate
What the cop message suggests	rename to `delete?`	Wrong: `delete` is a command
What the agent did (c51ebb3)	`Naming/PredicateMethod: Exclude: lib/n8n_api/resources/*.rb`	Cop off for 13 methods; smell still there
Correct fix (2c9d799, n8n-api#5)	return the record, or `nil`	The return value tells the caller something; Exclude removed

The real fix, from lib/n8n_api/resources/workflows.rb in 2c9d799:

# endpoint returns a body: return the record
def delete(id)
-  connection.delete("workflows/#{id}")
-  true
+  Types::Workflow.from_api(connection.delete("workflows/#{id}"))
end

# endpoint returns no body: return nil
def transfer(id, destination_project_id:)
  connection.put("workflows/#{id}/transfer", { destinationProjectId: destination_project_id })
-  true
+  nil
end

Reproduce

n/a: a lesson file is content. Lesson::Library resolves Naming/PredicateMethod to naming-predicate-method.yml with no code change.

Approach

Write plugins/os-sdlc/lint-lessons/naming-predicate-method.yml with the same seven keys as the existing lessons (cop, signal, sign_of, why, decision, correction, example). The lesson teaches this decision:

What the method is	Signal	Correction
A real query	The boolean answers a question	Rename it with `?`
A command that always returns the same value	`true` on every path	Return `nil`; return data only when a caller needs it. Tests assert the request sent or the state changed, never the return value
A method whose boolean depends on state	`true` or `false` varies	False positive. Report it; do not Exclude

Required work:

Add the lesson file. check: present cop: Naming/PredicateMethod in plugins/os-sdlc/lint-lessons/naming-predicate-method.yml

Acceptance criteria

Given the lesson file exists, when Lesson::Library loads the lesson directory, then it returns a lesson for Naming/PredicateMethod.
Given an offense from Naming/PredicateMethod, when any lint path renders its offense block, then the lesson text appears before the two smell questions.

Owner sketch

The command test is the core of the lesson. The n8n-api fix (2c9d799) swapped assert_equal true for assert_nil, and that test still passes if transfer sends nothing. The lesson teaches the test to assert the effect (Metz, "The Magic Tricks of Testing": incoming command, assert the direct public side effect).

# before
def transfer(id, destination_project_id:)
  connection.put("workflows/#{id}/transfer", { destinationProjectId: destination_project_id })
  true
end

def test_transfer_returns_true
  assert_equal true, client.workflows.transfer("abc123", destination_project_id: "p1")
end

# after
def transfer(id, destination_project_id:)
  connection.put("workflows/#{id}/transfer", { destinationProjectId: destination_project_id })
  nil
end

def test_transfer_sends_the_destination_project
  client.workflows.transfer("abc123", destination_project_id: "p1")
  assert_requested :put, "/workflows/abc123/transfer", body: { destinationProjectId: "p1" }
end

delete returning the record (2c9d799) is acceptable only when a caller uses the record.

Origin

Trigger: n8n-api fix-lints run, commit c51ebb3, Naming/PredicateMethod Exclude on 13 methods. Fixed in 2c9d799 (n8n-api#5).
Improvised this session: none.
Chain: Exclude added with no reason ← no lesson for the cop reached the programmer ← lint-lessons/ holds custom-cop lessons only. DESIGN: plugins/os-sdlc/lint-lessons/.
Root candidate: #989 (shipped) makes lessons reach every path; this ticket adds the lesson.
Where: plugins/os-sdlc/lint-lessons/
Session: 646d14c2-56a5-42d2-8798-b0b7fff69544
Transcript: /home/jared/.claude/projects/-home-jared-dev-cc-os/646d14c2-56a5-42d2-8798-b0b7fff69544.jsonl

VERDICT: CREATE
REASON: Real observed pain (n8n-api c51ebb3 added a Naming/PredicateMethod Exclude with no human approval). It fits the user pain-point policy, no existing lesson covers the cop, and #989 already routes lessons to every lint path. The fix is one content file.

## Problem `plugins/os-sdlc/lint-lessons/` has no lesson for `Naming/PredicateMethod`. In n8n-api, 13 resource methods always returned `true`. The cop flagged them, and a pipeline programmer turned the cop off for the whole directory (commit c51ebb3, jaredswanson/n8n-api#2). The cop message suggests a `?` rename, which was also wrong: these methods are commands, not questions. After #989 every lint path shows lessons, so a lesson for this cop reaches the next fixer. This is the pain point that started #989. | | Code | Result | | --- | --- | --- | | Offense | `def delete(id) … true end` | The cop says a method that returns only a boolean must be a predicate | | What the cop message suggests | rename to `delete?` | Wrong: `delete` is a command | | What the agent did (c51ebb3) | `Naming/PredicateMethod: Exclude: lib/n8n_api/resources/*.rb` | Cop off for 13 methods; smell still there | | Correct fix (2c9d799, n8n-api#5) | return the record, or `nil` | The return value tells the caller something; Exclude removed | The real fix, from `lib/n8n_api/resources/workflows.rb` in 2c9d799: ```ruby # endpoint returns a body: return the record def delete(id) - connection.delete("workflows/#{id}") - true + Types::Workflow.from_api(connection.delete("workflows/#{id}")) end # endpoint returns no body: return nil def transfer(id, destination_project_id:) connection.put("workflows/#{id}/transfer", { destinationProjectId: destination_project_id }) - true + nil end ``` ## Reproduce n/a: a lesson file is content. `Lesson::Library` resolves `Naming/PredicateMethod` to `naming-predicate-method.yml` with no code change. ## Approach Write `plugins/os-sdlc/lint-lessons/naming-predicate-method.yml` with the same seven keys as the existing lessons (`cop`, `signal`, `sign_of`, `why`, `decision`, `correction`, `example`). The lesson teaches this decision: | What the method is | Signal | Correction | | --- | --- | --- | | A real query | The boolean answers a question | Rename it with `?` | | A command that always returns the same value | `true` on every path | Return `nil`; return data only when a caller needs it. Tests assert the request sent or the state changed, never the return value | | A method whose boolean depends on state | `true` or `false` varies | False positive. Report it; do not Exclude | Required work: 1. Add the lesson file. check: present `cop: Naming/PredicateMethod` in `plugins/os-sdlc/lint-lessons/naming-predicate-method.yml` ## Acceptance criteria 1. Given the lesson file exists, when `Lesson::Library` loads the lesson directory, then it returns a lesson for `Naming/PredicateMethod`. 2. Given an offense from `Naming/PredicateMethod`, when any lint path renders its offense block, then the lesson text appears before the two smell questions. ## Owner sketch The command test is the core of the lesson. The n8n-api fix (2c9d799) swapped `assert_equal true` for `assert_nil`, and that test still passes if `transfer` sends nothing. The lesson teaches the test to assert the effect (Metz, "The Magic Tricks of Testing": incoming command, assert the direct public side effect). ```ruby # before def transfer(id, destination_project_id:) connection.put("workflows/#{id}/transfer", { destinationProjectId: destination_project_id }) true end def test_transfer_returns_true assert_equal true, client.workflows.transfer("abc123", destination_project_id: "p1") end # after def transfer(id, destination_project_id:) connection.put("workflows/#{id}/transfer", { destinationProjectId: destination_project_id }) nil end def test_transfer_sends_the_destination_project client.workflows.transfer("abc123", destination_project_id: "p1") assert_requested :put, "/workflows/abc123/transfer", body: { destinationProjectId: "p1" } end ``` `delete` returning the record (2c9d799) is acceptable only when a caller uses the record. ## Origin - Trigger: n8n-api fix-lints run, commit c51ebb3, Naming/PredicateMethod Exclude on 13 methods. Fixed in 2c9d799 (n8n-api#5). - Improvised this session: none. - Chain: Exclude added with no reason ← no lesson for the cop reached the programmer ← `lint-lessons/` holds custom-cop lessons only. `DESIGN`: `plugins/os-sdlc/lint-lessons/`. - Root candidate: #989 (shipped) makes lessons reach every path; this ticket adds the lesson. - Where: plugins/os-sdlc/lint-lessons/ - Session: 646d14c2-56a5-42d2-8798-b0b7fff69544 - Transcript: /home/jared/.claude/projects/-home-jared-dev-cc-os/646d14c2-56a5-42d2-8798-b0b7fff69544.jsonl VERDICT: CREATE REASON: Real observed pain (n8n-api c51ebb3 added a Naming/PredicateMethod Exclude with no human approval). It fits the user pain-point policy, no existing lesson covers the cop, and #989 already routes lessons to every lint path. The fix is one content file.

jared added the

plugin/os-sdlc

filed-by/agent

category/code-change

labels

2026-10-11 22:47:38 +00:00

jared referenced this issue

2026-10-11 22:47:49 +00:00

Give every lint-fixing skill and agent the lint lessons and a shared offense analysis #989

jared commented

2026-10-12 11:13:45 +00:00

Author

Owner

Resolution

Done: Added plugins/os-sdlc/lint-lessons/naming-predicate-method.yml (commit 0135e14c). The lesson teaches: query gets a ? name; command returns nil (data only when a caller needs it) and its tests assert the request or state change, never the return value; a state-dependent boolean is a false positive to report; never add an Exclude.

Evidence: Lesson::Library.new(dir: lint-lessons).lesson('Naming/PredicateMethod') returns an OsSdlc::Lesson. OffenseAnalysis.default.guidance(['Naming/PredicateMethod']) renders the lesson before the two smell questions (order check true). library_test, offense_analysis_test, offense_paths_test, relay_lesson_files_test: 29 runs, 0 failures. bin/refresh-plugins synchronized os-sdlc.

Follow-ups: Dropped: the n8n-api tests from 2c9d799 assert only the return value (assert_nil), not the request sent. Offered to file on the n8n-api tracker; the user did not choose it this session.

## Resolution **Done:** Added plugins/os-sdlc/lint-lessons/naming-predicate-method.yml (commit 0135e14c). The lesson teaches: query gets a ? name; command returns nil (data only when a caller needs it) and its tests assert the request or state change, never the return value; a state-dependent boolean is a false positive to report; never add an Exclude. **Evidence:** Lesson::Library.new(dir: lint-lessons).lesson('Naming/PredicateMethod') returns an OsSdlc::Lesson. OffenseAnalysis.default.guidance(['Naming/PredicateMethod']) renders the lesson before the two smell questions (order check true). library_test, offense_analysis_test, offense_paths_test, relay_lesson_files_test: 29 runs, 0 failures. bin/refresh-plugins synchronized os-sdlc. **Follow-ups:** Dropped: the n8n-api tests from 2c9d799 assert only the return value (assert_nil), not the request sent. Offered to file on the n8n-api tracker; the user did not choose it this session.

jared closed this issue

2026-10-12 11:13:45 +00:00

jared referenced this issue from a commit

2026-10-12 14:02:16 +00:00

feat(os-sdlc): lint lesson for Naming/PredicateMethod (#994)