feat: [GH-356] allow to use if with relations #357
Conversation
WalkthroughThe changes update the core Changes
Sequence Diagram(s)sequenceDiagram
participant Validator as CoreValidator
participant ValidationObj as Validation Object
Validator->>ValidationObj: Access "relations" property
alt relations is an Array
Validator->>Validator: Set relationType from array
else relations is an Object
Validator->>Validator: Set relationType from object
end
Validator->>Validator: Check for 'hasMany' & 'belongsTo' conditions
Validator-->>ValidationObj: Return validation result
sequenceDiagram
participant TestRunner as Test Runner
participant FakeModel as FakeModel
participant Validator as CoreValidator
TestRunner->>FakeModel: Trigger validation (with a specific condType)
FakeModel->>Validator: Execute _validateRelations for relationships
alt if function returns true (e.g., condType = 'gallery')
Validator->>FakeModel: Perform validation and report errors if any
else if function returns false (e.g., condType = 'chancuncha')
Validator->>FakeModel: Skip validation, no errors produced
end
FakeModel-->>TestRunner: Return validation outcome
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ixxvivxxi
changed the title
|
@ixxvivxxi Thank you for putting this together! Could you check of the failing test before we move ahead with this PR. |
|
@esbanarango Yes! I will fix it in a few days. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
package.json (1)
39-39: Fixed version specified for ember-cli-htmlbars.The dependency
ember-cli-htmlbarsis using a fixed version ("6.3.0") rather than a flexible version range (e.g.,^6.3.0). This could make future upgrades more difficult as it will not automatically accept compatible minor/patch versions.Consider using a caret range syntax for consistent dependency management:
- "ember-cli-htmlbars": "6.3.0", + "ember-cli-htmlbars": "^6.3.0",tests/dummy/config/ember-try.js (1)
3-3: Replaced dynamic channel URLs with fixed version scenarios.You've commented out
getChannelURLand dynamic test scenarios (release, beta, canary) in favor of fixed version scenarios. This approach provides more deterministic testing but may miss compatibility issues with upcoming Ember versions.Consider keeping at least one dynamic scenario (like release) to catch potential issues with the next version:
-// const getChannelURL = require('ember-source-channel-url'); +const getChannelURL = require('ember-source-channel-url'); // Later in the file: - // { - // name: 'ember-release', - // npm: { - // devDependencies: { - // 'ember-source': await getChannelURL('release'), - // 'ember-data': 'lts', - // }, - // }, - // }, + { + name: 'ember-release', + npm: { + devDependencies: { + 'ember-source': await getChannelURL('release'), + 'ember-data': 'lts', + }, + }, + },Also applies to: 46-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
README.md(1 hunks)package.json(1 hunks)tests/dummy/config/ember-try.js(2 hunks)tests/helpers/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/helpers/index.ts
🔇 Additional comments (4)
package.json (1)
110-111:❓ Verification inconclusive
Added resolutions field for @ember/test-waiters.
Adding a resolutions field to fix
@ember/test-waitersat version 3.1.0 suggests there might be a compatibility issue with this package. Resolutions are typically used to solve version conflicts or dependency issues.
🏁 Script executed:
#!/bin/bash # Check for known issues with @ember/test-waiters in GitHub issues gh search issues --repo "emberjs/ember.js" --state open "@ember/test-waiters" --json title,url,state | jq '.'Length of output: 5373
Double-check the necessity of the forced version for @ember/test-waiters
The update to set the resolution for
@ember/test-waitersto"3.1.0"inpackage.json(lines 110–111) appears to be a precaution to avoid potential dependency conflicts. A search for known issues in the Ember.js repository did not return any direct problems related to this package version. However, since resolutions are usually applied to overcome version conflicts, please verify that this change is intentional and that no side effects occur (e.g., run your full test suite).
- Location:
package.json, lines 110–111- Observation: No open issues related to
@ember/test-waitersat 3.1.0 were found in the Ember.js GitHub issues.- Action: Confirm that the forced resolution is necessary and that all tests pass after this change.
README.md (2)
23-26: LGTM! Added clear compatibility information.The new compatibility section clearly documents the version requirements for this addon, which is excellent for users to understand what versions of Ember.js and Ember Data are supported.
25-25:✅ Verification successful
Verify compatibility with ember-source 6.2.
The README states compatibility with ember-source up to version 6.2, but it would be good to confirm this has been thoroughly tested.
🏁 Script executed:
#!/bin/bash # Check if there are test scenarios for the stated maximum compatibility version grep -A 5 "ember-6.2" tests/dummy/config/ember-try.jsLength of output: 230
Compatibility verification for ember-source 6.2 confirmed
The test configuration in
tests/dummy/config/ember-try.jsincludes a scenario namedember-6.2-and-ember-data-5.3, which explicitly sets'ember-source': '~6.2.0'. This confirms that compatibility with ember-source 6.2 has been thoroughly tested as stated in the README.tests/dummy/config/ember-try.js (1)
29-33: Updated test scenarios to match compatibility requirements.The test scenarios have been updated to include specific versions mentioned in the README's compatibility section. This ensures that the addon is properly tested against the supported Ember versions.
Also applies to: 38-42
|
@esbanarango Hi! I was only able to get working tests up to |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
59-64: Review Updated Test Scenarios in the MatrixThe CI matrix now includes the new scenarios
"ember-lts-5.12-and-ember-data-5.3"and"ember-6.2-and-ember-data-5.3", which are aligned with the versions where tests currently pass. This adjustment avoids running tests for failing or unstable Ember versions while ensuring core functionality is verified.
65-67: Clarify Commented-Out ScenariosThe older scenarios (
ember-release,ember-beta, andember-canary) have been left commented out. Please ensure that this decision is documented—either via an inline comment or commit message—so that future maintainers understand these are being retained for potential reactivation once the issues are resolved.
|
@esbanarango Hi! I need to understand what happened, because yesterday I could run all tests, but today when I remove node_modules I get an error. |
Yeah something went off after locking ED version 🤔 . Check the output of this tests failing: https://github.com/esbanarango/ember-model-validator/actions/runs/14117761374/job/39552213633?pr=357 |
Summary by CodeRabbit
ember-sourceandember-data.package.jsonfor improved compatibility and performance.ember-try.jsfor better version management.