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

Split: whitespace and empty space #256

Closed
wants to merge 2 commits into from

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Oct 23, 2022

Issue
  1. Comma-separated text usually includes a space after comma. When widgets splits by comma, we then have two attributes - those starting with space and those without. Whitespace at the beginning and end is not shown, hence having it will never be intentional, imho. Thus it needs to be removed.
  2. If empty strings are considered missing then in case the string is empty, values of all new variables should be shown as missing. Current implementation instead adds an indicator variable for missing strings, which is probably not even used, (r used in case of two consecutive delimiters?)
  3. However, instead of fixing 2, I'd prefer to interpret empty strings differently in this context: they are empty because they do not contain any of the values into which the string is split. I think this makes more sense.
Description of changes

Fix 1, fix 2 via 3.

Includes
  • Code changes
  • Tests

@@ -34,7 +34,7 @@ def _create_simple_corpus(self) -> None:
[
["foo,"],
["bar,baz "],
["foo,bar"],
["foo, bar"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bar with leading space must be the same as bar without.

@codecov-commenter
Copy link

Codecov Report

Base: 40.42% // Head: 40.42% // No change to project coverage 👍

Coverage data is based on head (f839f3c) compared to base (6b5c959).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #256   +/-   ##
=======================================
  Coverage   40.42%   40.42%           
=======================================
  Files           7        7           
  Lines        1269     1269           
=======================================
  Hits          513      513           
  Misses        756      756           
Impacted Files Coverage Δ
orangecontrib/prototypes/widgets/owsplit.py 95.74% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@janezd
Copy link
Contributor Author

janezd commented Oct 23, 2022

Now I'm scared. Seriously. I already fixed this in (still to be merged) #253. Now that I see it, I remember.

@janezd janezd closed this Oct 23, 2022
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.

2 participants