feat: add support for @deprecated directive #2140
Conversation
WalkthroughAdds GraphQL deprecation handling to SDL→Proto generation: detects deprecated fields and enum values (including interface-sourced directives), emits deprecation comments and Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
protographic/src/sdl-to-proto-visitor.ts (3)
1152-1179: Emit trimmed deprecation reason and normalize field options emission
- Reason: handle whitespace-only reasons (avoid emitting empty comments).
- Options: build a single bracket group [opt1, opt2] instead of joining pre-bracketed strings. This makes adding future options safe and proto-compliant.
Apply this diff:
- const deprecationInfo = this.fieldIsDeprecated(field, [...type.getInterfaces()]); + const deprecationInfo = this.fieldIsDeprecated(field, [...type.getInterfaces()]); @@ - if (deprecationInfo.deprecated && deprecationInfo.reason && deprecationInfo.reason.length > 0) { - this.protoText.push( - ...this.formatComment(`Field ${protoFieldName} is deprecated: ${deprecationInfo.reason}`, 1), - ); - } + const reasonText = deprecationInfo.reason?.trim(); + if (deprecationInfo.deprecated && reasonText) { + this.protoText.push( + ...this.formatComment(`Field ${protoFieldName} is deprecated: ${reasonText}`, 1), + ); + } - const fieldOptions = []; - if (deprecationInfo.deprecated) { - fieldOptions.push(` [deprecated = true]`); - } + const fieldOptionParts: string[] = []; + if (deprecationInfo.deprecated) { + fieldOptionParts.push('deprecated = true'); + } + const optionsSuffix = fieldOptionParts.length ? ` [${fieldOptionParts.join(', ')}]` : ''; @@ - this.protoText.push( - ` repeated ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${fieldOptions.join(' ')};`, - ); + this.protoText.push( + ` repeated ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${optionsSuffix};`, + ); } else { - this.protoText.push(` ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${fieldOptions.join(' ')};`); + this.protoText.push(` ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${optionsSuffix};`); }
1291-1318: Mirror object-field deprecation refinements for input fieldsSame rationale: trim the reason and construct a single options bracket group.
Apply this diff:
- const deprecationInfo = this.fieldIsDeprecated(field, []); + const deprecationInfo = this.fieldIsDeprecated(field, []); @@ - if (deprecationInfo.deprecated && deprecationInfo.reason && deprecationInfo.reason.length > 0) { - this.protoText.push( - ...this.formatComment(`Field ${protoFieldName} is deprecated: ${deprecationInfo.reason}`, 1), - ); - } + const reasonText = deprecationInfo.reason?.trim(); + if (deprecationInfo.deprecated && reasonText) { + this.protoText.push( + ...this.formatComment(`Field ${protoFieldName} is deprecated: ${reasonText}`, 1), + ); + } - const fieldOptions = []; - if (deprecationInfo.deprecated) { - fieldOptions.push(` [deprecated = true]`); - } + const fieldOptionParts: string[] = []; + if (deprecationInfo.deprecated) { + fieldOptionParts.push('deprecated = true'); + } + const optionsSuffix = fieldOptionParts.length ? ` [${fieldOptionParts.join(', ')}]` : ''; @@ - this.protoText.push( - ` repeated ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${fieldOptions.join(' ')};`, - ); + this.protoText.push( + ` repeated ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${optionsSuffix};`, + ); } else { - this.protoText.push(` ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${fieldOptions.join(' ')};`); + this.protoText.push(` ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${optionsSuffix};`); }
1505-1535: Enum: trim reason and normalize value options emissionAlign with field/input handling. This prevents whitespace-only comments and ensures single bracket for multiple options.
Apply this diff:
- if (deprecationInfo.deprecated && (deprecationInfo.reason?.length ?? 0) > 0) { - this.protoText.push( - ...this.formatComment(`Enum value ${protoEnumValue} is deprecated: ${deprecationInfo.reason}`, 1), - ); - } + const reasonText = deprecationInfo.reason?.trim(); + if (deprecationInfo.deprecated && reasonText) { + this.protoText.push( + ...this.formatComment(`Enum value ${protoEnumValue} is deprecated: ${reasonText}`, 1), + ); + } @@ - const fieldOptions = []; - if (deprecationInfo.deprecated) { - fieldOptions.push(` [deprecated = true]`); - } - - this.protoText.push(` ${protoEnumValue} = ${valueNumber}${fieldOptions.join(' ')};`); + const valueOptionParts: string[] = []; + if (deprecationInfo.deprecated) { + valueOptionParts.push('deprecated = true'); + } + const valueOptionsSuffix = valueOptionParts.length ? ` [${valueOptionParts.join(', ')}]` : ''; + this.protoText.push(` ${protoEnumValue} = ${valueNumber}${valueOptionsSuffix};`);
🧹 Nitpick comments (4)
protographic/src/sdl-to-proto-visitor.ts (1)
1152-1179: Reduce duplication: extract a tiny helper to emit field lines with optionsYou’re repeating similar blocks (compute reason/comment; build options; write field/enum line). Consider a private helper like emitDeprecatedComment(name, reason, indent) and buildOptionsSuffix(deprecated: boolean): string to DRY these call sites.
Also applies to: 1291-1318, 1505-1535
protographic/tests/sdl-to-proto/12-directive.test.ts (3)
172-203: Add a test for whitespace-only reasonsYou validate empty-string reasons; also cover whitespace-only (e.g., " ") to ensure comments are suppressed after trimming.
Example to add:
it('should not write a comment if the reason is whitespace-only', () => { const sdl = ` enum UserRole { ADMIN @deprecated(reason: " ") USER } `; const { proto: protoText } = compileGraphQLToProto(sdl); expectValidProto(protoText); expect(protoText).toContain('USER_ROLE_ADMIN = 1 [deprecated = true];'); expect(protoText).not.toContain('// Enum value USER_ROLE_ADMIN is deprecated:'); });
6-35: Consider adding an object-field test for repeated fieldsValidate that repeated fields also get [deprecated = true] and the comment, e.g., posts: [String!]! @deprecated(reason: "Use articles").
Example:
it('should mark repeated object fields as deprecated', () => { const sdl = ` type User { posts: [String!]! @deprecated(reason: "Use articles") } `; const { proto: protoText } = compileGraphQLToProto(sdl); expectValidProto(protoText); expect(protoText).toMatch(/repeated string posts = 1 \[deprecated = true\];/); expect(protoText).toMatch(/\/\/ Field posts is deprecated: Use articles/); });
326-365: Consider a conflict test with multiple interfaces providing reasonsIf a type implements multiple interfaces that deprecate the same field with different reasons (and the field itself lacks a reason), assert that the first interface (by declaration order) is chosen. This solidifies precedence behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
protographic/src/sdl-to-proto-visitor.ts(9 hunks)protographic/tests/sdl-to-proto/12-directive.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
protographic/tests/sdl-to-proto/12-directive.test.ts (2)
protographic/src/index.ts (1)
compileGraphQLToProto(53-79)protographic/tests/util.ts (1)
expectValidProto(29-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
protographic/src/sdl-to-proto-visitor.ts (1)
6-9: Imports for deprecation handling look correctAdding GraphQLEnumValue, GraphQLInputField, and Kind is appropriate for directive/AST inspection.
Also applies to: 26-26
protographic/tests/sdl-to-proto/12-directive.test.ts (1)
5-468: Great coverage of deprecation scenariosThe suite thoroughly validates field, interface, enum, and input-field deprecations, including wrappers and nested inputs. Snapshots match the intended proto output.
Router image scan passed✅ No security vulnerabilities found in image: |
Router image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
demo/pkg/subgraphs/projects/src/schema.graphql (2)
135-135: Also deprecate the corresponding input field for consistencyTo fully discourage estimations end-to-end, consider deprecating the input counterpart in TaskInput. This keeps API ergonomics consistent and showcases input field deprecation support added in this PR.
Suggested change (outside this hunk):
input TaskInput { # ... estimatedHours: Float @deprecated(reason: "No more estimations!") }If you prefer, I can draft a follow-up patch touching the TaskInput block as well.
135-135: Optional: Add an enum deprecation example in the demo schemaSince tests cover enum value deprecations, the demo could include one example (e.g., deprecating a rarely used status) to better illustrate the feature in generated artifacts. Only if it doesn’t confuse demo users.
Example (outside this hunk):
enum TaskStatus { TODO IN_PROGRESS REVIEW @deprecated(reason: "Use IN_PROGRESS or COMPLETED") COMPLETED BLOCKED }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
demo/pkg/subgraphs/projects/generated/service.pb.gois excluded by!**/*.pb.go,!**/generated/**demo/pkg/subgraphs/projects/generated/service.protois excluded by!**/generated/**
📒 Files selected for processing (1)
demo/pkg/subgraphs/projects/src/schema.graphql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
demo/pkg/subgraphs/projects/src/schema.graphql (1)
135-135: Correct use of @deprecated on an object field — LGTMValid GraphQL syntax; reason is a proper String. No conflicting interface field exists, so no ambiguity in composed schema.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
protographic/src/sdl-to-proto-visitor.ts (3)
1153-1180: Future-proof field options formatting (single bracket, no leading-space tokens)Current approach stores options as pre-bracketed, leading-space strings and joins them. This doesn't scale to multiple options (you’d get duplicated brackets or double spaces). Build a proper options suffix once, with a single bracket block, and store only assignments in the array.
Apply this diff:
- const deprecationInfo = this.fieldIsDeprecated(field, [...type.getInterfaces()]); + const deprecationInfo = this.fieldIsDeprecated(field, [...type.getInterfaces()]); @@ - if (deprecationInfo.deprecated && deprecationInfo.reason && deprecationInfo.reason.length > 0) { + if (deprecationInfo.deprecated && deprecationInfo.reason && deprecationInfo.reason.length > 0) { this.protoText.push( ...this.formatComment(`Field ${protoFieldName} is deprecated: ${deprecationInfo.reason}`, 1), ); } - const fieldOptions = []; - if (deprecationInfo.deprecated) { - fieldOptions.push(` [deprecated = true]`); - } + const fieldOptions: string[] = []; + if (deprecationInfo.deprecated) { + fieldOptions.push('deprecated = true'); + } + const optionsSuffix = fieldOptions.length ? ` [${fieldOptions.join(', ')}]` : ''; if (fieldType.isRepeated) { - this.protoText.push( - ` repeated ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${fieldOptions.join(' ')};`, - ); + this.protoText.push( + ` repeated ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${optionsSuffix};`, + ); } else { - this.protoText.push(` ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${fieldOptions.join(' ')};`); + this.protoText.push(` ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${optionsSuffix};`); }
1301-1328: Apply the same options formatting improvement to input fieldsMirror the single-bracket options construction to avoid formatting issues when adding more options later.
Apply this diff:
- const deprecationInfo = this.fieldIsDeprecated(field, []); + const deprecationInfo = this.fieldIsDeprecated(field, []); @@ - if (deprecationInfo.deprecated && deprecationInfo.reason && deprecationInfo.reason.length > 0) { + if (deprecationInfo.deprecated && deprecationInfo.reason && deprecationInfo.reason.length > 0) { this.protoText.push( ...this.formatComment(`Field ${protoFieldName} is deprecated: ${deprecationInfo.reason}`, 1), ); } - const fieldOptions = []; - if (deprecationInfo.deprecated) { - fieldOptions.push(` [deprecated = true]`); - } + const fieldOptions: string[] = []; + if (deprecationInfo.deprecated) { + fieldOptions.push('deprecated = true'); + } + const optionsSuffix = fieldOptions.length ? ` [${fieldOptions.join(', ')}]` : ''; @@ - this.protoText.push( - ` repeated ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${fieldOptions.join(' ')};`, - ); + this.protoText.push( + ` repeated ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${optionsSuffix};`, + ); } else { - this.protoText.push(` ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${fieldOptions.join(' ')};`); + this.protoText.push(` ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${optionsSuffix};`); }
1514-1545: Unify enum value options formatting with fieldsUse a single bracketed option list for enums too. This keeps formatting consistent across fields and enum values and prevents double brackets if more options are added later.
Apply this diff:
- const deprecationInfo = this.enumValueIsDeprecated(value); + const deprecationInfo = this.enumValueIsDeprecated(value); @@ - if (deprecationInfo.deprecated && (deprecationInfo.reason?.length ?? 0) > 0) { + if (deprecationInfo.deprecated && (deprecationInfo.reason?.length ?? 0) > 0) { this.protoText.push( ...this.formatComment(`Enum value ${protoEnumValue} is deprecated: ${deprecationInfo.reason}`, 1), ); } @@ - const fieldOptions = []; - if (deprecationInfo.deprecated) { - fieldOptions.push(` [deprecated = true]`); - } - - this.protoText.push(` ${protoEnumValue} = ${valueNumber}${fieldOptions.join(' ')};`); + const valueOptions: string[] = []; + if (deprecationInfo.deprecated) { + valueOptions.push('deprecated = true'); + } + const optionsSuffix = valueOptions.length ? ` [${valueOptions.join(', ')}]` : ''; + + this.protoText.push(` ${protoEnumValue} = ${valueNumber}${optionsSuffix};`);
♻️ Duplicate comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1193-1232: Fix precedence: prefer first non-empty, trimmed reason; else fall back; also trim returned reasonAs written, if a field has @deprecated(reason: " ") the function will select that directive and return deprecated with no reason, never checking interface-provided non-empty reasons. This contradicts the docstring (“field-level reason takes precedence; otherwise the first interface with a non-empty reason wins”). Also, the returned reason for fields is not trimmed (enums are).
Apply this diff:
private fieldIsDeprecated( field: GraphQLField<any, any> | GraphQLInputField, interfaces: GraphQLInterfaceType[], ): { deprecated: boolean; reason?: string } { @@ - // Find the first directive with a non-empty (trimmed) reason: field first, then interfaces - const firstWithReason = deprecatedDirectives.find( - (d) => d.arguments?.find((a) => a.name.value === 'reason')?.value, - ); - - if (!firstWithReason) { - return { deprecated: true }; - } - - const reason = firstWithReason.arguments?.find((a) => a.name.value === 'reason')?.value; - if (this.isNonEmptyStringValueNode(reason)) { - return { deprecated: true, reason: reason.value }; - } - - return { deprecated: true }; + // Find the first directive with a non-empty (trimmed) reason: field first, then interfaces + const firstNonEmptyReasonDirective = deprecatedDirectives.find((d) => { + const node = d.arguments?.find((a) => a.name.value === 'reason')?.value as ConstValueNode | undefined; + return this.isNonEmptyStringValueNode(node); + }); + + if (firstNonEmptyReasonDirective) { + const reasonNode = firstNonEmptyReasonDirective.arguments?.find((a) => a.name.value === 'reason')?.value as + | ConstValueNode + | undefined; + if (this.isNonEmptyStringValueNode(reasonNode)) { + return { deprecated: true, reason: reasonNode.value.trim() }; + } + } + + // No non-empty reason found, but at least one @deprecated directive exists + return { deprecated: true }; }
🧹 Nitpick comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1234-1245: Enum deprecation: good handling; consider parity fallback when no AST existsThe enum path trims reasons and emits the option/comment correctly. If you ever support programmatically-built schemas without AST nodes, you could optionally fall back to value.deprecationReason when deprecatedDirective is missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
protographic/src/sdl-to-proto-visitor.ts(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
protographic/src/sdl-to-proto-visitor.ts (2)
3-9: LGTM: new imports are correct and usedConstValueNode, GraphQLEnumValue, GraphQLInputField, and Kind are all needed for the deprecation parsing helpers and are used below.
Also applies to: 27-29
1247-1254: LGTM: robust helper for non-empty string valuesThe Kind guard plus trim-based emptiness check is correct and re-usable.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1224-1264: Fix precedence and type narrowing when resolving field deprecation reasonTwo issues:
- Precedence: if the field has a deprecated directive with an empty reason, you currently stop there and lose a non-empty interface reason. We should pick the first non-empty (trimmed) reason with field-level precedence.
- Types:
.filter((d) => d !== undefined)doesn’t narrow the type for TS; accessingd.argumentswithout a type-guard will fail under stricter settings.Adjust the find predicate to use
isNonEmptyStringValueNodeand add proper type guards. Also trim the returned reason to mirror enum handling.Apply this diff:
private fieldIsDeprecated( field: GraphQLField<any, any> | GraphQLInputField, interfaces: GraphQLInterfaceType[], ): { deprecated: boolean; reason?: string } { - const allFieldsRefs = [ - field, - ...interfaces.map((iface) => iface.getFields()[field.name]).filter((f) => f !== undefined), - ]; + const allFieldsRefs = [ + field, + ...interfaces + .map((iface) => iface.getFields()[field.name]) + .filter((f): f is GraphQLField<any, any> => f != null), + ]; - const deprecatedDirectives = allFieldsRefs - .map((f) => f.astNode?.directives?.find((d) => d.name.value === 'deprecated')) - .filter((d) => d !== undefined); + const deprecatedDirectives = allFieldsRefs + .map((f) => f.astNode?.directives?.find((d) => d.name.value === 'deprecated')) + .filter((d): d is DirectiveNode => d !== undefined); if (deprecatedDirectives.length === 0) { return { deprecated: false }; } - // Find the first directive with a non-empty (trimmed) reason: field first, then interfaces - const firstWithReason = deprecatedDirectives.find( - (d) => d.arguments?.find((a) => a.name.value === 'reason')?.value, - ); + // Find the first directive with a non-empty (trimmed) reason: field first, then interfaces + const firstWithReason = deprecatedDirectives.find((d) => { + const node = d.arguments?.find((a) => a.name.value === 'reason')?.value; + return this.isNonEmptyStringValueNode(node); + }); if (!firstWithReason) { return { deprecated: true }; } const reason = firstWithReason.arguments?.find((a) => a.name.value === 'reason')?.value; - if (this.isNonEmptyStringValueNode(reason)) { - return { deprecated: true, reason: reason.value }; + if (this.isNonEmptyStringValueNode(reason)) { + return { deprecated: true, reason: reason.value.trim() }; } return { deprecated: true }; }Would you like me to add a test asserting that an empty field reason falls back to the first non-empty interface reason?
🧹 Nitpick comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1545-1576: Nit: avoid shadowinglockDatawithin the same scopeInside the loop you redeclare
const lockData = this.lockManager.getLockData();despite havinglockDataearlier in the function. Rename or reuse for clarity.Example minimal change:
- const lockData = this.lockManager.getLockData(); + const currentLockData = this.lockManager.getLockData(); let valueNumber = 0; - if (lockData.enums[type.name] && lockData.enums[type.name].fields[value.name]) { - valueNumber = lockData.enums[type.name].fields[value.name]; + if (currentLockData.enums[type.name] && currentLockData.enums[type.name].fields[value.name]) { + valueNumber = currentLockData.enums[type.name].fields[value.name]; } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
protographic/src/sdl-to-proto-visitor.ts(12 hunks)protographic/tests/sdl-to-proto/12-directive.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protographic/tests/sdl-to-proto/12-directive.test.ts
🧰 Additional context used
🪛 GitHub Actions: Protographic CI
protographic/src/sdl-to-proto-visitor.ts
[warning] 1-1: Prettier formatting issue detected in src/sdl-to-proto-visitor.ts. Run 'prettier --write' to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
protographic/src/sdl-to-proto-visitor.ts (7)
1-29: New GraphQL imports are correct and usedAll newly added imports (ConstValueNode, GraphQLEnumValue, GraphQLInputField, Kind) are referenced below. Good.
520-528: Whitespace normalization step looks goodTrimming leading/trailing blank lines before emission and inserting a single separator only when protoText has content keeps output tidy and deterministic.
538-559: Helper to trim empty lines: LGTMEfficient, readable, and correctly handles the all-empty case.
1184-1211: Proto emission for deprecated object fields: LGTM
- Emits an explanatory comment only when a non-empty reason exists.
- Appends
[deprecated = true]consistently for both repeated and non-repeated fields.- Keeps numbering intact.
Nice.
1265-1276: Enum deprecation handling: LGTM
- Reason is trimmed before emission.
- Emits
[deprecated = true]independent of reason presence.Matches field behavior.
1279-1285: String value node helper: LGTMGood reuse point and aligns with GraphQL Kind.STRING.
1332-1359: Input field deprecation propagation: LGTMSymmetric to object fields, with correct comment/option emission and no interface consideration.
…466-add-support-for-deprecated-directive
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1244-1253: Trim field deprecation reason to avoid whitespace artifacts (align with enum logic)Currently, the returned reason uses the original node value without trimming. This can leak leading/trailing spaces into comments. Enum handling already trims; mirror that here.
Apply this diff:
- return { deprecated: true, reason: reasons[0]?.value }; + // Normalize whitespace to avoid leaking leading/trailing spaces into comments + const reasonNode = reasons[0] as StringValueNode; + const text = reasonNode.value.trim(); + return { deprecated: true, reason: text };
🧹 Nitpick comments (6)
protographic/src/sdl-to-proto-visitor.ts (4)
1196-1207: Avoid double-spacing in options by building a single suffixStoring fragments with a leading space and then joining can produce double spaces if more options are added later. Build a single suffix instead.
Apply this diff:
- const fieldOptions = []; - if (deprecationInfo.deprecated) { - fieldOptions.push(` [deprecated = true]`); - } + const optionsSuffix = deprecationInfo.deprecated ? ' [deprecated = true]' : ''; @@ - this.protoText.push( - ` repeated ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${fieldOptions.join(' ')};`, - ); + this.protoText.push(` repeated ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${optionsSuffix};`); @@ - this.protoText.push(` ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${fieldOptions.join(' ')};`); + this.protoText.push(` ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${optionsSuffix};`);
1322-1347: Mirror the options suffix simplification for input fieldsSame rationale as object fields: compute a single options suffix to avoid spacing quirks and ease future extensions.
Apply this diff:
- const deprecationInfo = this.fieldIsDeprecated(field, []); + const deprecationInfo = this.fieldIsDeprecated(field, []); @@ - const fieldOptions = []; - if (deprecationInfo.deprecated) { - fieldOptions.push(` [deprecated = true]`); - } + const optionsSuffix = deprecationInfo.deprecated ? ' [deprecated = true]' : ''; @@ - this.protoText.push( - ` repeated ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${fieldOptions.join(' ')};`, - ); + this.protoText.push(` repeated ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${optionsSuffix};`); @@ - this.protoText.push(` ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${fieldOptions.join(' ')};`); + this.protoText.push(` ${fieldType.typeName} ${protoFieldName} = ${fieldNumber}${optionsSuffix};`);
1533-1543: Enum deprecation handling looks solid; minor spacing nit in optionsLogic is correct: comment only when reason is non-empty, and always emit the [deprecated = true] flag when deprecated. Consider the same single-suffix approach here to keep spacing uniform.
Apply this diff:
- const fieldOptions = []; - if (deprecationInfo.deprecated) { - fieldOptions.push(` [deprecated = true]`); - } - - this.protoText.push(` ${protoEnumValue} = ${valueNumber}${fieldOptions.join(' ')};`); + const optionsSuffix = deprecationInfo.deprecated ? ' [deprecated = true]' : ''; + this.protoText.push(` ${protoEnumValue} = ${valueNumber}${optionsSuffix};`);Also applies to: 1556-1562
1051-1055: Use Kind.STRING for consistency when checking directive argument kindsElsewhere you use Kind.STRING. For consistency and to avoid stringly-typed comparisons, prefer Kind here too.
Apply this diff:
- if (fieldsArg && fieldsArg.value.kind === 'StringValue') { + if (fieldsArg && fieldsArg.value.kind === Kind.STRING) { const stringValue = fieldsArg.value as StringValueNode; return stringValue.value; }Also applies to: 27-29
protographic/tests/sdl-to-proto/12-directive.test.ts (2)
168-168: Fix test title typo: remove duplicate “should”Minor polish for readability.
Apply this diff:
- it('should should ignore empty reason on field when interface has a reason', () => { + it('should ignore empty reason on field when interface has a reason', () => {
1-500: Test coverage is strong; consider two small additions
- Add a case where an object field is deprecated with a whitespace-only reason and no interface, asserting that only [deprecated = true] is emitted without a comment.
- Add a case where an enum value has a whitespace-only reason (not empty), also asserting no comment.
I can draft these tests if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
protographic/src/sdl-to-proto-visitor.ts(12 hunks)protographic/tests/sdl-to-proto/12-directive.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
protographic/tests/sdl-to-proto/12-directive.test.ts (2)
protographic/src/index.ts (1)
compileGraphQLToProto(53-79)protographic/tests/util.ts (1)
expectValidProto(29-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
protographic/src/sdl-to-proto-visitor.ts (4)
520-526: Nice addition: deterministic whitespace trimming around generated sectionsThe trimEmptyLines utility and its integration into visit() keeps the proto output tidy and stable, while still inserting a single separator when additional content exists. This should reduce snapshot churn.
Also applies to: 536-556
1182-1182: Good precedence and source resolution for deprecation (field first, then interfaces)Pulling interface field directives as a fallback for object fields is the right call and matches GraphQL expectations. Field-level reason precedence is preserved by order.
1255-1266: Enum reason parsing and trimming: LGTMUsing Kind.STRING and trimming ensures comments are emitted only when meaningful. Consistent with tests.
1273-1275: Helper reads well and centralizes reason validationisNonEmptyStringValueNode keeps the checks tidy and reusable.
Noroth
deleted the
ludwig/eng-7466-add-support-for-deprecated-directive
branch
Summary by CodeRabbit
New Features
Tests
Chores
Checklist