Skip to content

Conversation

KManolov3
Copy link
Owner

No description provided.

@coveralls
Copy link

coveralls commented Feb 16, 2024

Pull Request Test Coverage Report for Build 7985647537

Details

  • -1 of 40 (97.5%) changed or added relevant lines in 1 file are covered.
  • 360 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.2%) to 64.806%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/core/src/util/renderer.ts 39 40 97.5%
Files with Coverage Reduction New Missed Lines %
packages/angular-material/node_modules/@jsonforms/core/src/util/renderer.ts 36 78.95%
packages/angular/node_modules/@jsonforms/core/src/util/renderer.ts 36 78.95%
packages/angular-test/node_modules/@jsonforms/core/src/util/renderer.ts 36 78.95%
packages/examples/node_modules/@jsonforms/core/src/util/renderer.ts 36 78.95%
packages/examples-react/node_modules/@jsonforms/core/src/util/renderer.ts 36 78.95%
packages/material-renderers/node_modules/@jsonforms/core/src/util/renderer.ts 36 78.95%
packages/react/node_modules/@jsonforms/core/src/util/renderer.ts 36 78.95%
packages/vanilla-renderers/node_modules/@jsonforms/core/src/util/renderer.ts 36 78.95%
packages/vue/node_modules/@jsonforms/core/src/util/renderer.ts 36 78.95%
packages/vue-vanilla/node_modules/@jsonforms/core/src/util/renderer.ts 36 78.95%
Totals Coverage Status
Change from base Build 7927830359: 0.2%
Covered Lines: 18465
Relevant Lines: 21540

💛 - Coveralls

rootSchema,
rootSchema,
schemaPath,
// TODO: Why was this previously schemaPath

Choose a reason for hiding this comment

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

It was schemaPath because the logic for getting the next heigher schema path was delegated to isRequiredInParent. Maybe that was why we were skipping every other parent.

has(get(currentSchema, 'properties'), prevSegments[i])
(has(currentSchema, prevSegments[i]) ||
(has(currentSchema, 'properties') &&
has(get(currentSchema, 'properties'), prevSegments[i])))

Choose a reason for hiding this comment

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

Since now we are adding /items in the schema path, then maybe there should be another condition for it. For example would a path like this cause a problem: #/properties/object/properties/array/items/properties/nestedArray/items/properties/propertyName?

}

const nextHigherSchemaPath = nextHigherSchemaSegments.join('/');
const nextHigherSchema = Resolve.schema(schema, nextHigherSchemaPath, schema);

Choose a reason for hiding this comment

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

I don't think using schema instead of rootSchema is better here. From what I see, it is true that we don't need the rootSchema, but if a new feature is added that relies on it, maybe it would cause a problem.

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.

3 participants