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

Fix: remove head error table #89

Merged
merged 10 commits into from
Dec 11, 2023
Merged

Conversation

vanhry
Copy link
Contributor

@vanhry vanhry commented Dec 6, 2023

Issue #88

Before submitting a PR please answer:

  • Was it necessary to update README? Is README updated?
  • Are unit tests added for new logic?

Changes:

  • df_error_head_n variable added to control nrow for error data.frame

Copy link
Member

@jakubnowicki jakubnowicki left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. I have a few comments. IMO, showing all error values should be optional and controlled using a parameter. Imagine a scenario where a dataset with 1,000,000 rows fails on all of them.
The solution I recommend here is that by default, the behavior stays the same. I can see a few options here to consider - a parameter to set the number of displayed rows and a parameter to show all rows. The latter probably should add some pagination to the table. Also, you will need to adjust the header, as it clearly says that this is only a sample. If you show all errors, than the word "sample" should be removed.
Besides that, two small things:

  1. Please bump the package version. We follow the X.X.X.900X convention for the development version, so simply change the last X.
  2. The change is going to belong to the development version, so please follow my suggestion in the changelog.

NEWS.md Outdated
Comment on lines 6 to 8

# data.validator 0.2.1

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
# data.validator 0.2.1

Copy link
Member

Choose a reason for hiding this comment

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

This change belongs to the development version.

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (ab89645) 83.65% compared to head (5a7961c) 83.80%.
Report is 2 commits behind head on main.

Files Patch % Lines
R/semantic_report_constructors.R 82.35% 3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #89      +/-   ##
==========================================
+ Coverage   83.65%   83.80%   +0.15%     
==========================================
  Files           5        5              
  Lines         410      420      +10     
==========================================
+ Hits          343      352       +9     
- Misses         67       68       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jakubnowicki jakubnowicki left a comment

Choose a reason for hiding this comment

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

LGTM

@jakubnowicki jakubnowicki merged commit 5bf9e30 into main Dec 11, 2023
5 checks passed
@jakubnowicki jakubnowicki deleted the fix/remove-head-error-table branch December 11, 2023 09:42
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