fix : Allow schema as a schema path name #8798 #9734
Conversation
|
@vkarpov15 I made a PR for the issue #8798 , but I don´t understand why did the CI build failed, I looked at the details but I don't understand why it failed, looks like a connection, but I can´t understand at all, could you please help me understand better. |
| @@ -101,17 +101,17 @@ function Document(obj, fields, skipId, options) { | |||
| throw new ObjectParameterError(obj, 'obj', 'Document'); | |||
| } | |||
|
|
|||
| const schema = this.schema; | |||
| const reserved_schema = this.schema; | |||
There was a problem hiding this comment.
This unfortunately won't work in the case that schema is a path name. In that case, this.schema may be a string or something else, so this assignment won't work.
There was a problem hiding this comment.
Well I tried what you told me to change all the places in lib/document.js where we use this.schema there was actually 45 references but when you run the test it gives you 3 more errors.
And actually, the first error that makes reference to the path name stays the same, is there a way to see what does the first test is expecting? AssertionError [ERR_ASSERTION]: Missing expected exception.
Because I guess I renamed the schema property where it wasn't needed, and that brings me the 3 more errors, so when I try to compile the app, it says this...
I don't know if I'm explaining myself enough so you can understand me, sorry if I am making this more complicated. I have been trying many ways but can not find the correct one. Hope you can explain me a little more.
Cheers!
There was a problem hiding this comment.
Do you recommend me to push the changes so you can review them? I am kind of lost now, I didn't push them because well they didn't pass the test.
There was a problem hiding this comment.
Given that there's 45 references, this change may be a little more complex than I'd imagined. It likely isn't a good first issue, there's a lot of small refactoring to be done. You'd need to change each of those 45 references. Is that still something you're interested in working on?
There was a problem hiding this comment.
Yes, of course, I don't wanna give up, just if you could help me a little more understanding the code, is being kind of difficult for me to understand it.
I already change the name on the 45 references, but it crashes with other documents as I can understand because it gives 3 more errors, which are the 2), 3), and 4) of the image above. It can not read the property emit and options from files model.js and query.js
And when I tried to test it with another project it gives the error that can not find the property [scopeSymbol] from undefined, so I am definitely changing the variable, where maybe it is not supposed to be changed.
There was a problem hiding this comment.
The error message in (2) is indicative of the fact that you changed this.schema in a static function on the Model class. Remember that Model.init() is a static function, not a method, so this.schema doesn't need to change here:
Line 1228 in 21f1f18
Remember that Document is a class, and Model inherits from Document. So Model has both a static property schema and an instance property schema from Document. The instance property needs to change, the static property doesn't.
|
I'm going to close this PR, it is long out of date and unlikely to end up in a mergeable state. |
Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. The two fields below are mandatory.
If you're making a change to documentation, do not modify a
.htmlfile directly. Instead find the corresponding.pugfile or test case in thetest/docsdirectory.Summary
I am new at the open-source and I wanted to contribute to one of the libraries I use the most, look for a new good issue to solve and here I am. Mostly what it was about, was to allow a "schema" as a schema path name.
Examples
It fixs the issue #8798 actually I modify the test, at schema.test.js line 1441, so that way I make sure the solution was correct.