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: ignore None type categories #1632

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

desobolevsky
Copy link

Actual PR for #1325
Duplicate for #1416, but since this PR seems to be outdated and no longer update/rebased to the latest version, doing this.

@desobolevsky
Copy link
Author

Hey, @fabclmnt! Don't wanna be intrusive, but could you please take a look at my PR? :)
The tests seem to fail for some external reasons not connected to changes at the code.
Thanks in advance!

Copy link

codecov bot commented Aug 19, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.22%. Comparing base (1314e0d) to head (c4055c7).
Report is 11 commits behind head on develop.

Files Patch % Lines
...g/report/structure/variables/render_categorical.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1632      +/-   ##
===========================================
+ Coverage    90.06%   90.22%   +0.16%     
===========================================
  Files          196      197       +1     
  Lines         6439     6453      +14     
===========================================
+ Hits          5799     5822      +23     
+ Misses         640      631       -9     
Flag Coverage Δ
py3.8-ubuntu-22.04-pandas 90.22% <50.00%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@alexbarros
Copy link
Contributor

sry for the delay @desobolevsky, I will be having a look at this week. My questions with this PR is more related to the root cause of the problem, which is still not clear. This workaround may lead to silent errors, since the user is expecting their unicode categories to be categorized as Other Symbol, just like 😀 and 🔥.

Also, when creating a fix for specific use case like this we expect the addition of a unit test tests/issues/test_issue1325.py to ensure the issue is fixed and we don't run into it again in the future.

@fabclmnt fabclmnt self-requested a review August 26, 2024 11:12
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