fix(adk): remove false from isEmpty in checkMetadataUnchanged#90
fix(adk): remove false from isEmpty in checkMetadataUnchanged#90ThePlenkov merged 2 commits intomainfrom
Conversation
`false` was treated as "empty-like", causing isSubsetMatch(false, true)
to return true — silently skipping saves when boolean flags are toggled
from true to false (e.g., lowercase, fixPointArithmetic, final).
Now only null, undefined, "", [], and {} are empty-like. An explicit
`false` is compared against SAP's value as a real change.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 7d3d903
☁️ Nx Cloud last updated this comment at |
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Stale comment still lists false as an empty-like value after it was explicitly removed
Line 867 contains the comment // Both empty-like → match (handles null/undefined/""/false/[]/{} which still lists false as an empty-like value. However, this PR explicitly removed false from the isEmpty function (line 858-862) and added a comment (line 856-857) clarifying that false is NOT empty-like. The stale comment contradicts the new behavior and could mislead future developers into thinking isEmpty(false) returns true when it now returns false.
(Refers to lines 867-868)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
Fixes ADK metadata “unchanged” detection so explicit boolean false is no longer treated as empty-like, preventing saves from being silently skipped when flags toggle from true → false.
Changes:
- Update
checkMetadataUnchanged()empty-like detection to excludefalse. - Adjust inline documentation/examples to reflect that
falsemust be compared as a real value.
| // "Empty-like" values (null, undefined, "", [], {}) are treated | ||
| // as equivalent, since SAP normalizes defaults differently than | ||
| // abapGit serialization (e.g., style "" → "00", missing → false). | ||
| // abapGit serialization (e.g., style "" → "00", missing → undefined). | ||
| // Note: `false` is NOT empty-like — an explicit `false` must be | ||
| // compared against SAP's value to detect boolean flag toggles. |
There was a problem hiding this comment.
The inline documentation is now inconsistent with the updated isEmpty semantics. In this same function, the comment above the isEmpty(local) && isEmpty(sap) branch still mentions false as an empty-like value, and earlier in save() the comment still lists false as empty-like (around lines ~486-487). Please update those comments to match the new behavior (only null/undefined/""/[]/{} are empty-like, and false is compared as a real value).
There was a problem hiding this comment.
@copilot apply changes based on this feedback
falsewas treated as "empty-like", causing isSubsetMatch(false, true) to return true — silently skipping saves when boolean flags are toggled from true to false (e.g., lowercase, fixPointArithmetic, final).Now only null, undefined, "", [], and {} are empty-like. An explicit
falseis compared against SAP's value as a real change.Generated with Devin
Summary by CodeRabbit