fix(generation): Make order deterministic#177
Conversation
JS code generation relies on non-deprecated attributes being visited before deprecated ones for constant name generation, but this was never enforced. This causes tests to fail under Windows, where files are apparently listed in a different order. Therefore, this PR makes the iteration order in JS code generation deterministic by sorting attributes by: 1. What name the constant should have; 2. Deprecation (non-deprecated first). Python generation is unchanged; the problem isn't so critical there because constant names have already been generated and memoized at that point.
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Fixes
- Make order deterministic ([#177](https://github.com/getsentry/sentry-conventions/pull/177))If none of the above apply, you can opt out of this check by adding |
| const attributePath = path.join(attributesDir, file); | ||
| const attributeJson = JSON.parse(fs.readFileSync(attributePath, 'utf-8')) as AttributeJson; | ||
| const isDeprecated = !!attributeJson.deprecation; | ||
| const constantName = getConstantName(attributeJson.key, isDeprecated); | ||
|
|
||
| allAttributes.push({ | ||
| allAttributesPartial.push({ | ||
| file, |
There was a problem hiding this comment.
Bug: Calls to getConstantName are missing the required isDeprecated argument, which will cause a TypeScript compilation error.
(Severity: Critical 0.80 | Confidence: 1.00)
🔍 Detailed Analysis
The function signature for getConstantName was changed to require two parameters: key and isDeprecated. However, two call sites within the generateMetadataDict function were not updated to reflect this change. The calls getConstantName(key) and getConstantName(aliasKey) are both missing the required isDeprecated boolean argument. This discrepancy will cause a TypeScript compilation error during the build process (e.g., when running yarn generate), preventing the successful generation of attribute metadata.
💡 Suggested Fix
Update the two calls to getConstantName inside generateMetadataDict to provide the required second argument, isDeprecated. For the first call, you can destructure isDeprecated from attributeJson. For the alias mapping, the correct deprecation status needs to be determined and passed.
🤖 Prompt for AI Agent
Fix this bug. In scripts/generate_attributes.ts at lines 57-62: The function signature
for `getConstantName` was changed to require two parameters: `key` and `isDeprecated`.
However, two call sites within the `generateMetadataDict` function were not updated to
reflect this change. The calls `getConstantName(key)` and `getConstantName(aliasKey)`
are both missing the required `isDeprecated` boolean argument. This discrepancy will
cause a TypeScript compilation error during the build process (e.g., when running `yarn
generate`), preventing the successful generation of attribute metadata.
Did we get this right? 👍 / 👎 to inform future reviews.
|
Thanks for taking care of this @loewenheim, I thought about doing this previously but at least on macos the files were always sorted in the same way, so I didn't think this would be an issue. |
|
Thank you very much @lcian! |
Description
JS code generation relies on non-deprecated attributes being visited before deprecated ones for constant name generation, but this was never enforced. This causes tests to fail under Windows, where files are apparently listed in a different order.
Therefore, this PR makes the iteration order in JS code generation deterministic by sorting attributes by:
Python generation is unchanged; the problem isn't so critical there because constant names have already been generated and memoized at that point.
PR Checklist
yarn testand verified that the tests pass.yarn generate && yarn formatto generate and format code and docs.