Clean up lint warnings in core/ec* #8409
Conversation
tcobbs-bentley
changed the title
tcobbs-bentley
requested review from
a team,
ColinKerr,
StefanApfel-Bentley,
christophermlawson and
khanaffan
as code owners
| await this.validate(relClass!); | ||
| await this.validate(expectDefined(relClass)); | ||
| } catch(e: any) { | ||
| await (relClass! as ECClass as MutableClass).setBaseClass(baseClass); | ||
| await (expectDefined(relClass) as ECClass as MutableClass).setBaseClass(baseClass); |
There was a problem hiding this comment.
@christophermlawson what is the expected behavior when the item key doesn't resolve to a relationship class?
This should be fixed properly rather than with this hack and the behavior should be documented and tested
There was a problem hiding this comment.
@ColinKerr This would be unexpected, so I think an exception would be appropriate here.
| const enumeration = await context.targetSchema.lookupItem(itemKey) as MutableEnumeration; | ||
| if(change.difference.type !== undefined) { | ||
| throw new Error(`The Enumeration ${itemKey.name} has an incompatible type. It must be "${primitiveTypeToString(enumeration.type!)}", not "${change.difference.type}".`); | ||
| throw new Error(`The Enumeration ${itemKey.name} has an incompatible type. It must be "${enumeration.type ? primitiveTypeToString(enumeration.type) : "<unknown>"}", not "${change.difference.type}".`); |
There was a problem hiding this comment.
Why is the underlying Enumeration._type property possibly undefined? Any valid enumeration MUST have a type set so we should not force callers to test if the type is null.
There was a problem hiding this comment.
@ColinKerr The constructor also has this as optional. It's internal so we can change this. Should we require the parameter or have a default (int or string)?
| return; | ||
|
|
||
| return new Diagnostics.IncompatibleValueTypePropertyOverride(property, [property.class.fullName, property.name, baseClass.fullName, primitiveTypeToString(baseType), primitiveTypeToString(primitiveType!)]); | ||
| return new Diagnostics.IncompatibleValueTypePropertyOverride(property, [property.class.fullName, property.name, baseClass.fullName, primitiveTypeToString(baseType), primitiveTypeToString(expectDefined(primitiveType))]); |
There was a problem hiding this comment.
This code should be rewritten so the call back isn't reaching out of its scope to get primitiveType. It would be clearer if primitiveType was passed in as the overrideType of something like that indicating that this the type of the property that is trying to override a property from the base class. It would also avoid the need for expectDefined here.
There was a problem hiding this comment.
I discovered that the requirement for ! or expectDefined was caused by what I feel is a compiler bug. (The compiler for some reason thinks that primitiveType might be undefined in callback, even though it's a const that has already been verified to be defined.) Introducing another const removed the need for either ! or expectDefined, so I made that change. Note that this doesn't address your actual comment here, but it does provide what I feel is a better fix to get rid of the !, and your comment no longer seems to be in the scope of this PR.
| const maxCandidate = candidates.sort(this.compareSchemaKeyByVersion)[candidates.length - 1]; | ||
| const alias = this.getSchemaAlias(maxCandidate.schemaText!); | ||
| // Note: if maxCandidate.schemaText is undefined, the "" passed to getSchemaAlias will throw an ECSchemaError. | ||
| const alias = this.getSchemaAlias(maxCandidate.schemaText ?? ""); |
There was a problem hiding this comment.
This is problematic. findEligibleSchemaKeys always fills out the schemaText. Some code uses the schemaText and other code loads it all over again. It looks like we may load the text from disk more than once and in some cases we may hold onto the text long after we need it (when we construct the schema with a SchemaFileKey)
This should be rationalized and fixed
There was a problem hiding this comment.
@ColinKerr I'm assuming you are referring to the other locaters (json and xml)? Looks like we may read it twice. The StubSchemaXmlFileLocater looks ok to me.
| private loadSchemaReferenceSync(ref: SchemaReferenceProps): void { | ||
| const schemaKey = new SchemaKey(ref.name, ECVersion.fromString(ref.version)); | ||
| const refSchema = this._context.getSchemaSync(schemaKey, SchemaMatchType.LatestWriteCompatible); | ||
| if (undefined === this._schema) { |
There was a problem hiding this comment.
Maybe it would be cleaner to rewrite this method to take the schema in as a parameter. As called _schema will always be defined but changing the signature makes the method's contract clear.
There was a problem hiding this comment.
I made that simple change. While doing so I noticed that it is being cast to MutableSchema without a guard. Should a new guard be added that verifies that it is an instance of MutableSchema and throw an exception if not?
|
This pull request is now in conflicts. Could you fix it @tcobbs-bentley? 🙏 |
Clean up the lint warnings in all the core/ec* directories.
!).expectDefinedandexpectNotNullfunctions when there isn't an obviously correct way to get rid of the!. As time passes, code usingexpectDefinedandexpectNotNullwill be updated to stop using it.@typescript-eslint/restrict-template-expressionswarning.