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

Task 42: recreating the PR #50

Merged
merged 6 commits into from
Sep 21, 2023

Conversation

nargis-sultani
Copy link
Contributor

No description provided.

@github-actions
Copy link

github-actions bot commented Sep 15, 2023

Coverage report

The coverage rate went from 82.26% to 93.34% ⬆️
The branch rate is 82%.

97.5% of new lines are covered.

Diff Coverage details (click to unfold)

src/tests/test_schema_functions.py

100% of new lines are covered (100% of the complete file).

src/validator/main.py

0% of new lines are covered (0% of the complete file).
Missing lines: 11, 30

src/validator/create_schemas.py

97.5% of new lines are covered (95.89% of the complete file).
Missing lines: 69


class TestValidate:
util = TestUtil()
phase1_schema = get_phase_1_schema_for_lei()
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add (positive and negative) tests with LEI value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

raise AttributeError(f"{str(check)}")

if hasattr(check, "id"):
check_id: str = check.id
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can add this logic to Ln 56 . Our checks should have id and name defined or else it should throw attribute error


def test_with_invalid_data(self):
result = validate_phases(pd.DataFrame(data=self.util.get_data({"ct_credit_product": ["989"]})))

Copy link
Contributor

Choose a reason for hiding this comment

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

Also add tests with LEI defined

@aharjati
Copy link
Contributor

This looks good to me. Probably need to have another pair of eyes to review this since this PR is using a bit of code from my WIP branch #32
Maybe @lchen-2101 or @guffee23 can take a quick look at this?

@lchen-2101
Copy link
Collaborator

Looks mostly fine for now; I'd like something in the future to refactor the the SchemaErrors processing part into more readable self documenting code. I'm a bit out of the loop on this now, but looks if phase 1 fails for any field, phase 2 wouldn't run for the entire file? Just checking this is functionally correct.

@aharjati
Copy link
Contributor

... looks if phase 1 fails for any field, phase 2 wouldn't run for the entire file? Just checking this is functionally correct.

@hkeeler , I believe this is still the plan?

@hkeeler
Copy link
Member

hkeeler commented Sep 21, 2023

Yes, that's correct. If phase 1 fails, we want it to stop processing.

@nargis-sultani
Copy link
Contributor Author

nargis-sultani commented Sep 21, 2023

@lchen-2101 Any changes needed to be done?

Copy link
Collaborator

@lchen-2101 lchen-2101 left a comment

Choose a reason for hiding this comment

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

LGTM, will merge; please make a ticket to refactor the the SchemaErrors processing part into more readable self documenting code.

@lchen-2101 lchen-2101 merged commit cb55a7f into main Sep 21, 2023
3 checks passed
@lchen-2101 lchen-2101 deleted the features/42_structure_validator_cli_output branch September 21, 2023 17:44
jcadam14 pushed a commit that referenced this pull request May 3, 2024
Co-authored-by: Nargis Sultani <nargis.sultani@cfpb.gov>
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.

None yet

4 participants