-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(update): automatically identify pk and fks into repository layer #150
Conversation
Codecov Report
@@ Coverage Diff @@
## main #150 +/- ##
==========================================
+ Coverage 86.28% 87.41% +1.12%
==========================================
Files 25 25
Lines 423 437 +14
==========================================
+ Hits 365 382 +17
+ Misses 58 55 -3
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@danielsous the current implementation is not handling multiples IDs: const Project =
entity('Project', {
id1: id(String),
id2: id(String),
name: field(String),
}) |
@@ -8,7 +8,9 @@ class <%- props.name.pascalCase %>Repository extends Repository { | |||
super({ | |||
entity: <%- props.name.pascalCase %>, | |||
table: "<%- props.table %>", | |||
knex: connection | |||
ids: [<% for (const [index, value] of Object.entries(props.primaryKeyFields)) { %><%if(index != 0) { %>, <% } %>'<%= value %>'<% } %>], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the code seems duplicated in other files.
I think this should be refactored
fs.rmSync(path.resolve(process.cwd(), `${projectName}`), { recursive: true }) | ||
}) | ||
|
||
it('Should I create a new repository with foreign key', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a test without the foreing key
@danielsous great contrib! having support for entities relationship is one of the most important feature for now. just a few points: |
I don't think it is necessary to explicitly inform PKs ( |
1 to 1 scenario:Testing with two entities with relationship between then: const User =
entity('User', {
id: id(String),
nickname: field(String),
})
const Project =
entity('Project', {
id: id(String),
user: field(User),
}) The repository ( |
1 to Many scenario:Testing with two entities with relationship between then: const User =
entity('User', {
id: id(String),
nickname: field(String),
})
const Project =
entity('Project', {
id: id(String),
user: field([User]), // <----- Many users
}) When Generating Entities
Generating Repositories
New: C:\Users\David\Projects\herbs-project\src\infra\data\repositories\userRepository.js
C:\Users\David\Projects\herbs-cli\node_modules\gluegun\build\index.js:15
throw up;
^
TypeError: Cannot read properties of undefined (reading 'meta')
at C:\Users\David\Projects\herbs-cli\src\generators\src\infra\database\repositories\repositories.js:13:51
at Array.map (<anonymous>)
at generateForeignKeysField (C:\Users\David\Projects\herbs-cli\src\generators\src\infra\database\repositories\repositories.js:10:25)
at generateRepositories (C:\Users\David\Projects\herbs-cli\src\generators\src\infra\database\repositories\repositories.js:40:45) |
@danielsous I think your PR is very close to the effort started by this PR #144 created by @endersoncosta regarding issue #145 . You guys should join forces to finish it. As I said, this feature is veeery import for the project |
Fixes #26
Proposed Changes
Readiness Checklist
Author/Contributor
Reviewing Maintainer
breaking
if this is a large fundamental changeautomation
,bug
,documentation
,enhancement
,infrastructure
, orperformance