-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
feature: add removeVirtual(path)
function to schema
#12920
Conversation
lib/schema.js
Outdated
if (this.virtuals[name] == null && !this.nested[name]) { | ||
throw new MongooseError('Attempting to remove virtual path that does not exist.'); | ||
} | ||
if (this.nested[name]) { |
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.
Please add a test case that shows when this code path would be executed. Not sure when this would happen.
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 is my mistake. I thought that if a virtual for whatever reason was nested it would go there. Evidently, it seems that you can't nest a virtual as when I attempted to write a test to do so it was not registered in the schema anywhere.
lib/schema.js
Outdated
function _deleteVirtualPath(schema, name) { | ||
const virtuals = schema.virtuals; | ||
if (virtuals[name]) delete virtuals[name]; | ||
else throw new MongooseError('Attempting to delete a a virtaul path that does not exist'); // You should never see this message |
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.
Multiple typos in error message. Also what is with the comment?
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.
the removeVirtual()
function is checking for that already. So for else block to execute, a user would have to be calling the _deleteVirtualPath
function directly which would be circumventing the process.
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.
Ah ok, so the issue is that this check is completely unnecessary, as is the helper function. I removed both in af20f0e.
No description provided.