Conversation
giortzisg
left a comment
giortzisg
left a comment
There was a problem hiding this comment.
Nice change,
left some comments for minimizing context usage, but overall looks good
AGENTS.md
Outdated
| - Follow the existing formatting rules from `.php-cs-fixer.dist.php`: Symfony | ||
| style, short arrays, ordered imports, yoda conditions, and 4-space | ||
| indentation. |
There was a problem hiding this comment.
it's probably better to just point the agent to the file, without specifying the rules:
- in case they ever change, we would need to maintain the AGENTS.md file as well
- we add duplicate things to the context if the model loads the file
AGENTS.md
Outdated
| - The main entry points are: | ||
| - `src/SentryBundle.php`: registers compiler passes. | ||
| - `src/DependencyInjection/Configuration.php`: defines the config tree. | ||
| - `src/DependencyInjection/SentryExtension.php`: turns config into container | ||
| definitions and parameters. | ||
| - `src/Resources/config/services.yaml`: defines base services and listener | ||
| tags. | ||
| - `src/aliases.php`: selects version-specific classes by runtime feature | ||
| detection. |
There was a problem hiding this comment.
We also had a similar conversation in sentry-go.
It probably makes sense to let the agent explore the codebase, to gather more info by itself. Aliases seems the most important here, but it's already mentioned under compatibility rules.
AGENTS.md
Outdated
| - Config changes usually require synchronized updates in all of these places: | ||
| - `src/DependencyInjection/Configuration.php` | ||
| - `src/DependencyInjection/SentryExtension.php` | ||
| - `src/Resources/config/services.yaml` | ||
| - `src/Resources/config/schema/sentry-1.0.xsd` | ||
| - `tests/DependencyInjection/Fixtures/{yml,php,xml}` | ||
| - `tests/DependencyInjection/ConfigurationTest.php` | ||
| - `tests/DependencyInjection/SentryExtensionTest.php` |
There was a problem hiding this comment.
Also this is probably something that the model can infer, from either exploring or from composer checks.
AGENTS.md
Outdated
| - Local verification commands: | ||
| - `composer install` | ||
| - `composer cs-check` | ||
| - `composer phpstan` | ||
| - `composer psalm` | ||
| - `composer tests` | ||
| - `composer check` | ||
| - `composer phpstan` runs PHPStan with `512M` because the default local PHP | ||
| memory limit can be too low for this repo. | ||
| - `php-cs-fixer` is the formatting tool in this repo. If formatting fails, run | ||
| `composer cs-fix` and then rerun `composer check`. | ||
| - Before committing, run `composer check`. |
There was a problem hiding this comment.
I feel we can just keep only the last bullet point for a pre-commit check. The models should be good enough to handle composer commands.
There was a problem hiding this comment.
I will overhaul this. I noticed that this file is missing the instruction to always perform formatting and linter checks after editing files
| - `CONTRIBUTING.md` states that new code should include tests and notes that | ||
| style checkers require PHP 7.4+ locally, but shipped code still has to comply | ||
| with the package's PHP `7.2` baseline. |
There was a problem hiding this comment.
this is already implied from compatibility
AGENTS.md
Outdated
| ## CI Notes | ||
|
|
||
| - `.github/workflows/tests.yaml` runs a PHP/Symfony matrix and also includes | ||
| jobs that remove optional packages before running tests. | ||
| - `.github/workflows/static-analysis.yaml` runs PHP-CS-Fixer, PHPStan, and | ||
| Psalm on a recent PHP version. Check the workflow file for the exact version | ||
| currently used in CI. | ||
| - A single local green run is necessary but not sufficient. Avoid changes that | ||
| only work when optional packages are present. |
There was a problem hiding this comment.
Do you feel this section is required? Does the model ever wait for CI after making a change? This seems to be worth it for a more automated workflow.
There was a problem hiding this comment.
It doesn't wait on the CI changes, but in my local test it tried to make all tools work on all PHP versions, which ultimately failed and produced a lot of helper code.
Telling it that the CI uses the matrix but the linters and static analysis tools run on one version resolved the problem without having to remind it to read the CI file
There was a problem hiding this comment.
The last bullet point is a bit redundant though
2 participants
Adds
AGENTS.mdandCLAUDE.mdto the repository to enable Agents to understand the codebase better and to know which tools to run and when