-
Notifications
You must be signed in to change notification settings - Fork 137
Solve #376: clashing in case of more classes with the same name #384
Conversation
sorry, but could you re-target it to the r6/master (on my fork)? PS: please read the CONTRIBUTING file and how to structure commits for PR's, thanks (i mean especially to squash as needed, thanks) |
Yes, I can. But I'm using the current version 5.9.0, and I'm very interested to have this feature in ver 5.
Where's the CONTRIBUTING file? |
to your right, when you start an pull request, otherwise here
that is the reason why v6 is currently not on the offical npm & offical repo(this one here), because only @szokodiakos can give the rights to this but v6 is release under here are some things my fork has, but the offical repo (this one) dosnt: hasezoey#1 |
I'm confused! Hi @Ben305, please could you clarify me some points:
|
yes he is a maintainer (/collaborator), but currently dosnt do any code changes & Accepting PR's (look at the still open ones, and how "old" they are), only currently accepted changes are documentation changes, hes not often doing much here in the last time PS: look at #99 PPS: what the transfer would do / should do / can do: typegoose/typegoose#1 (comment) |
btw i am now looking through your pr a bit, and it seems you modified many documentation changes (Readme, etc) that are "not related" to the actual solving of "clash in case of more classes with same name", so could you spread it in multiple PR'S? |
Unfortunately some format option caused diff also in rows that I didn't edit. |
I've just pushed @szokodiakos to grant you maintainer permission as asked by Ben in #99 |
yes i saw |
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 with "typeAlias" for
Lines 81 to 88 in aaec1bf
/** | |
* Get the Class for a given Document | |
* @param document The Document | |
*/ | |
export function getClassForDocument(document: mongoose.Document): any { | |
const modelName = (document.constructor as mongoose.Model<typeof document>).modelName; | |
return constructors[modelName]; | |
} |
for reference
https://github.com/szokodiakos/typegoose/blob/master/test/tests/getClassForDocument.test.ts
|
nope, cant find it |
It's true. I'm going to understand this function and I'll write a test. |
trying to help: this function gets the class from the the supplied document, if it can be found in the class "array" (not in 6.0.0 its a map) |
- Tested getClassForDocument() for a model created using typeAlias
@@ -45,12 +46,18 @@ export function suite() { | |||
expect(userReflectedType).to.not.equals(CarType); | |||
}); | |||
|
|||
it(`should return correct class type for document also using typeAlias`, async () => { | |||
const event = new EventModel(); |
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.
but are you sure that it is just this model, when only having one named "Event"?
-> please add to be sure an dummy Event(Class) and make it have different properties, thanks
PS: shouldnt this test be in typeAlias.test.ts? (im not fully sure)
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.
Yes, I'm sure. As you asked, I've added another model named Event with different properties and typeAlias.
the listed commit, is from version 6.0.0, so "extending typegoose" is deprecated, and is not used PS: if you got redirected to a wrong URL, sorry, my bad, fixed it |
Could you apply your commit to this project that reproduces my error with class name conflict? |
(didnt test or run it yet, but wanted to say some things:) the commit i listed here, makes differing models, with "same name", but only if you add an option for "collection" or a "customName" into |
i just run your thing, changed it to v6 syntax, and no build errors, should i look at something different? |
Have you launched |
So this proves that your version doesn't fix the case reproduced here https://github.com/fabioformosa/typegoose-validation-error and explained in #376 |
as you can see, i deleted my comment, because i did something wrong, will report again |
i ran npm build: passes npm test: both passes give me write permission to your test repo, and i will give the change to you (in a different branch, to compare) |
I've just given it to you |
pushed to |
I got, but |
no https://www.npmjs.com/package/@hasezoey/typegoose/v/6.0.0-25 | PS: i use PPS: what is it for an error? |
Ok npm problem solved. I have a question: Why did you add |
because i prefer const bar = new BarModel({ fieldOne: 'fieldOne', fieldTwo: 'fieldTwo' } as Bar);
return await FooModel.create({ bar: bar } as Foo); over const bar = new Bar();
bar.fieldOne = 'fieldOne';
bar.fieldTwo = 'fieldTwo';
const foo = new Foo();
foo.bar = bar;
const fooModel: InstanceType<Foo> = new FooModel(foo);
return await fooModel.save(); the main difference i mean with it: const bar = new Bar();
bar.fieldOne = 'fieldOne';
bar.fieldTwo = 'fieldTwo'; |
Ok, test passed. |
does this solve your question? TL;DR: if |
this can be closed because it will not be included in this repo anymore |
What does the Feature do
This PR avoids clashing in validation check, if more classes has the same name.
typeAlias
attribute in@prop()
is possibile to disambiguate name of types used as nested object.typeAlias
attribute inGetModelForClassOption
is possible to disambiguate models with the same class name.Please, look at:
typeAlias
tests/typeAlias.test.ts
Related Issues