Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Omit extra data in schemaUtils #4139

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

Draft
MarekBodingerBA wants to merge 4 commits into rjsf-team:main
base: main
Choose a base branch
Loading
from bratislava:omit-extra-data

Conversation

@MarekBodingerBA
Copy link
Contributor

@MarekBodingerBA MarekBodingerBA commented Mar 22, 2024 •
edited
Loading

Reasons for making this change

fixes #4081

For now I implemented the draft version (code and markdown documentation is not finished yet). Would you be able to look if everything seems alright with the progress so far?

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

Copy link
Member

@heath-freenome heath-freenome left a comment

Choose a reason for hiding this comment

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

Looks like your tests are failing

Comment on lines +2 to +6
import _pick from 'lodash/pick';

import _get from 'lodash/get';
import _isEmpty from 'lodash/isEmpty';
import toPathSchema from './toPathSchema';
Copy link
Member

@heath-freenome heath-freenome Mar 22, 2024

Choose a reason for hiding this comment

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

Suggested change
import _pick from 'lodash/pick';
import _get from 'lodash/get';
import_isEmptyfrom'lodash/isEmpty';
import toPathSchema from './toPathSchema';
import _pick from 'lodash/pick';
import_getfrom'lodash/get';
import _isEmpty from 'lodash/isEmpty';
import toPathSchema from './toPathSchema';

return data as T;
}

export default function omitExtraData<
Copy link
Member

@heath-freenome heath-freenome Mar 22, 2024

Choose a reason for hiding this comment

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

Consider adding JSDoc

 

 - getFieldNames tests are taken from Form.test.jsx and modified
 - getUsedFormData tests are taken from Form.test.jsx and modified, originally they contained schemas that weren't necessary
 - some extra tests for omitExtraData are created
Copy link
Member

@MarekBodingerBA There is one failing tests, and you also have to get the test coverage back up to 100% given the new code you wrote

Copy link
Contributor Author

@MarekBodingerBA There is one failing tests, and you also have to get the test coverage back up to 100% given the new code you wrote

Of course, I am still working on it, it's a draft afterall . :)

Copy link
Member

@MarekBodingerBA There is one failing tests, and you also have to get the test coverage back up to 100% given the new code you wrote

Of course, I am still working on it, it's a draft afterall . :)

Awesome! how do you feel it is going? Are you needing any help?

Copy link
Contributor Author

@MarekBodingerBA There is one failing tests, and you also have to get the test coverage back up to 100% given the new code you wrote

Of course, I am still working on it, it's a draft afterall . :)

Awesome! how do you feel it is going? Are you needing any help?

Actually, the failing test is a bug in the original code that I've only migrated and created a test that found this.

I would probably like to disable the test and create a separate issue for this (as there would be no regression). The thing is, that I find getFieldNames implementation a bit overblown (and buggy in this case) for what it does, but I think it is better do to in a separate issue and don't mix those two together.
heath-freenome reacted with thumbs up emoji

Copy link
Member

@MarekBodingerBA There is one failing tests, and you also have to get the test coverage back up to 100% given the new code you wrote

Of course, I am still working on it, it's a draft afterall . :)

Awesome! how do you feel it is going? Are you needing any help?

Actually, the failing test is a bug in the original code that I've only migrated and created a test that found this.

I would probably like to disable the test and create a separate issue for this (as there would be no regression). The thing is, that I find getFieldNames implementation a bit overblown (and buggy in this case) for what it does, but I think it is better do to in a separate issue and don't mix those two together.

That is fine, as long as we get that bug fixed soon after we merge this

Copy link
Member

@MarekBodingerBA How is this work going? There seem to be several new issues related to this feature and I would love to get this part refactored

Copy link
Member

@MarekBodinger FYI, I just merged a PR that will definitely force you to deal with the conflicts. How can I support you in getting this code merged?

Copy link
Contributor Author

Hi @heath-freenome,

sorry for not responding. I've got stuck with this pull request.

As far as I remember, it seems that in some cases the schema must be retrieved (as it always is in Form component where the original code) is. When I migrated the standalone implementation it started to fail for pretty ordinary schemas (as you can see in the test case). Then I started to digging into getFieldNames and I realized the best would be to completely rewrite the function, but wasn't really sure how to proceed with this safely.

Merge the code please, I am curious what's new. I would like to proceed in future with this, but don't have a capacity to do so now.

Copy link
Member

heath-freenome commented Jul 2, 2024 •
edited by nickgros
Loading

@MarekBodingerBA I can't quite merge the code as there are conflicts. Would you be willing to resolve them and comment out the one failing test. Then we can write an issue to deal with fixing the test and the bug associated with it. Also, a change was recently made to Form which is probably the source of the conflicts and you may need to fiddle with the code some to resolve them correctly.

Copy link
Member

@MarekBodinger Can you figure out if your PR is still even needed with all the features and optimizations we've added since it was created?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@heath-freenome heath-freenome heath-freenome left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Consider moving omit extra data functionality to schemaUtils

AltStyle によって変換されたページ (->オリジナル) /