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

Omit extra data in schemaUtils #4139

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MarekBodingerBA
Copy link
Contributor

@MarekBodingerBA MarekBodingerBA commented Mar 22, 2024

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

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 _isEmpty from 'lodash/isEmpty';
import toPathSchema from './toPathSchema';
import _pick from 'lodash/pick';
import _get from 'lodash/get';
import _isEmpty from 'lodash/isEmpty';
import toPathSchema from './toPathSchema';

packages/utils/src/schema/omitExtraData.ts Show resolved Hide resolved
packages/utils/src/schema/omitExtraData.ts Show resolved Hide resolved
return data as T;
}

export default function omitExtraData<
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding JSDoc

packages/utils/src/types.ts Show resolved Hide resolved
packages/utils/src/createSchemaUtils.ts Show resolved Hide resolved
 - 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
@heath-freenome
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

@MarekBodingerBA
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 . :)

@heath-freenome
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?

@MarekBodingerBA
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
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

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.

Consider moving omit extra data functionality to schemaUtils
3 participants