Skip to content
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: implemented getPopulatedDocs() #9766

Merged
merged 10 commits into from
Jan 9, 2021
Merged

feat: implemented getPopulatedDocs() #9766

merged 10 commits into from
Jan 9, 2021

Conversation

IslandRhythms
Copy link
Collaborator

No description provided.

lib/document.js Outdated
let result = [];

for (const key of index) {
let value = this.get(key);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this const instead of let

lib/document.js Outdated
@@ -3897,6 +3899,24 @@ Document.prototype.populate = function populate() {
return this;
};

/* Returns an array of all populated documents associated with the query. */
Document.prototype.$getPopulatedDocs = function $getPopulatedDocs() {
const index = (Object.keys(this.$__.populated));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this variable to keys or something that's more indicative of how this is used.

lib/document.js Outdated
result.push(value);

}
console.log(result);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this console.log()

lib/document.js Outdated

for (const key of index) {
let value = this.get(key);
if (Array.isArray(value)) result = result.concat(value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mongoose style guidelines generally say no one-line ifs. Curly braces {} around if always, so this should be:

if (Array.isArray(value)) {
  result = result.concat(value);
} else if (typeof value === 'object' && value !== null) {
  result.push(value);
}

});

return co(function*() {
const c = yield Child.create({ name: 'test' });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some issues with indentation here. Run npm run lint -- --fix and that should help fix these errors.


p.children; // [{ _id: '...', name: 'test' }]

assert.equal(p.$getPopulatedDocs().length, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be assert.equal(p.$getPopulatedDocs().length, 2); because both 'children' and 'child' are populated

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add this function to index.d.ts?

});

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the trailing whitespace

p.children; // [{ _id: '...', name: 'test' }]

assert.equal(p.$getPopulatedDocs().length, 2);
assert.equal(p.$getPopulatedDocs()[0], p.children[0]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also add a test that populatedDocs[1] is strictly equal to p.child

lib/document.js Outdated
@@ -3897,6 +3899,23 @@ Document.prototype.populate = function populate() {
return this;
};

/* Returns an array of all populated documents associated with the query. */
Document.prototype.$getPopulatedDocs = function $getPopulatedDocs() {
const pathway = (Object.keys(this.$__.populated));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not a fan of this name. Let's just use keys

assert.equal(p.$getPopulatedDocs().length, 2);
assert.equal(p.$getPopulatedDocs()[0], p.children[0]);
assert.equal(p.$getPopulatedDocs()[0].name, 'test');
assert.equal(p.$getPopulatedDocs()[1],p.child);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ESLint fails here because you need a space after ,. It's a bit pedantic, I know, but it helps with readability since code is typically read more times than it is written.

@vkarpov15 vkarpov15 changed the base branch from master to 5.12 January 9, 2021 17:58
@vkarpov15 vkarpov15 merged commit 848867c into 5.12 Jan 9, 2021
@Uzlopak Uzlopak deleted the #9702 branch June 14, 2022 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants