[infra] Agent skill: Apply dot shorthands#3164
Conversation
dcharkes
force-pushed
the
agent-skill-dot-shorthands
branch
from
e24d40c to
92a5d61
Compare
| super.language = Language.c, | ||
| ``` | ||
|
|
||
| Only do the refactoring if the contains the type modulo spacing. |
There was a problem hiding this comment.
Is this sentence missing a word or something?
|
|
||
| ```dart | ||
| // After | ||
| super.language = Language.c, |
There was a problem hiding this comment.
Why wouldn't this be super.language = .c,?
dcharkes
force-pushed
the
agent-skill-dot-shorthands
branch
from
92a5d61 to
5408f0e
Compare
dcharkes
force-pushed
the
agent-skill-dot-shorthands
branch
from
5408f0e to
f3aacae
Compare
dcharkes
changed the title
liamappelbe
left a comment
liamappelbe
left a comment
There was a problem hiding this comment.
My comments are all based on hand wavy notions of how LLMs think, so YMMV
|
|
||
| Apply Dart 3.7+ dot shorthands (member access shorthands) to simplify code where the context type is already known. | ||
|
|
||
| ## Constraints |
There was a problem hiding this comment.
Have you tried giving it a list of when it is allowed to use dot shorthands, rather than when it isn't? And just give it a blanket "Never use dot shorthands other than these approved cases...". Seems like you're playing whack-a-mole with the possible mistakes the AI can make here.
There was a problem hiding this comment.
Hm, I was hoping to get more creativity out of the LLM, in cases I hadn't thought about.
| super.language = .c, | ||
| ``` | ||
|
|
||
| Only do the refactoring if the receiver or parameter name contains the type modulo spacing. |
There was a problem hiding this comment.
This use of "modulo" is suuuuuper SWE specific. I wonder if it might confuse an AI trained on ordinary language.
| ``` | ||
|
|
||
| ```dart | ||
| // After, bad. (Type not clear from 'type' parameter) |
There was a problem hiding this comment.
My working model is that AIs have the reading-comprehension of a really sleepy human. I'd distinguish this case more than just a single "bad" here. Like, you could put negative examples in a separate heading. Or just update this comment to say in 3 different ways that this is a bad example.
// NEGATIVE EXAMPLE
// After (bad, don't do this).
super(type: .library); // BAD: Type unclear from param nameThere was a problem hiding this comment.
I've run the skill a couple of times, and I don't think this is necessary.
| super(outputType: .library); | ||
| ``` | ||
|
|
||
| However, only do this if the code quality doesn't suffer. |
There was a problem hiding this comment.
Hm, this seems to work. After adding this its behavior changed and when I ask it to explain why certain refactorings were done or not it referred to this.
|
|
||
| ### Collections and Lists | ||
|
|
||
| Maintain multiline formatting for better readability. |
There was a problem hiding this comment.
Interesting. I assumed the formatter overrides whatever we do here.
There was a problem hiding this comment.
No, we have turned on
formatter:
trailing_commas: preserveSo, the last comma matters.
| * Create a temporary markdown file (e.g., `SHORTHAND_PROGRESS.md`) containing a checklist of all files. | ||
| 3. **Iterative Refactoring**: Work through the files in small batches (1-3 files): | ||
| * **Identify**: Search for shorthand opportunities within the files that meet the **Constraints**. | ||
| * **Apply**: Replace `TypeName.memberName` with `.memberName`. |
There was a problem hiding this comment.
I've heard that these AIs do better if you ask them to criticise their own work. You could try adding a step here like: Review: Reread your change with fresh eyes, and make sure it matches the constraints. Revert if it makes the code unclear.
There was a problem hiding this comment.
Hm, but I want it to not do the change, make it work, and then revert things again. I want it to think before doing the change.
My hunch is that your suggestion might work slightly better but be significantly slower. I guess we'd need "evals" to check that. I've just been running it locally and look at the diff between runs manually.
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
Looks like a good starting point to iterate on. LGTM
3 participants
To play around with it start your favorite agent. The skills in the
.agent/directory are automatically picked up by multiple agents.