Skip to content
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

WIP: change path to JSON pointer #2168

Closed
wants to merge 1 commit into from

Conversation

LukasBoll
Copy link
Contributor

closes #2153

@netlify
Copy link

netlify bot commented Aug 3, 2023

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit caf7383
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/64e4b95692b01a00080688e4
😎 Deploy Preview https://deploy-preview-2168--jsonforms-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@coveralls
Copy link

coveralls commented Aug 21, 2023

Coverage Status

coverage: 84.511% (+0.04%) from 84.469% when pulling caf7383 on LukasBoll:pathToJSONPointer into c1686c1 on eclipsesource:master.

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor comments

@@ -159,7 +159,7 @@ export const deriveLabelForUISchemaElement = (
const i18nKeyPrefix = getI18nKeyPrefixBySchema(undefined, uischema);
const i18nKey =
typeof i18nKeyPrefix === 'string'
? `${i18nKeyPrefix}.label`
Copy link
Member

Choose a reason for hiding this comment

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

Here we want to keep the dots, right?

Comment on lines 31 to 42
let p2 = path2;
if (!isEmpty(path2) && !path2.startsWith('[') && !path2.startsWith('/')) {
p2 = '/' + path2;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove the square brackets handling here. They are not a valid feature of JSON pointers.

Comment on lines 36 to 39
if (isEmpty(path1)) {
return p2;
} else if (isEmpty(path2)) {
return p1;
return path1;
Copy link
Member

Choose a reason for hiding this comment

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

Will path2 which is handed in never start with a / already? The compose should have a JSDoc clearly declaring in which format it expects the handed over paths. Looking at this code it seems the path2 is rather a path segment.

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Please adapt migration document with a description of the breaking changes and how to fix them

@lucas-koehler
Copy link
Contributor

lucas-koehler commented Jun 14, 2024

Superseded by rebased and extended PR #2346 .
Thank your for your work on this! Your commit is also part of the new PR :)

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.

Square brackets in property name causes object to be destructured to many nested objects Unify path handling
4 participants