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] File: Disallow changing string columns to datetime #2050

Merged
merged 1 commit into from
Mar 3, 2017
Merged

[FIX] File: Disallow changing string columns to datetime #2050

merged 1 commit into from
Mar 3, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Feb 24, 2017

Issue

ValueError - could not convert string to float: '02.02.17'
https://sentry.io/biolab/orange3/issues/196743724/

Description of changes

Datetime option only if numerical.

Includes
  • Code changes
  • Tests
  • Documentation

@codecov-io
Copy link

codecov-io commented Feb 24, 2017

Codecov Report

Merging #2050 into master will decrease coverage by -1.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2050      +/-   ##
==========================================
- Coverage    70.7%   69.66%   -1.04%     
==========================================
  Files         315      315              
  Lines       53903    53903              
==========================================
- Hits        38111    37554     -557     
- Misses      15792    16349     +557

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b1813c...0f76144. Read the comment docs.

@jerneju
Copy link
Contributor Author

jerneju commented Feb 24, 2017

@jerneju
Copy link
Contributor Author

jerneju commented Feb 24, 2017

#1520, #1798

@ajdapretnar
Copy link
Contributor

This fixes the error nicely, but I was wondering whether we could provide the user with a better error message. Perhaps "Use ISO format for datetime variables (e.g. 2017-02-27)"? Say in a form of a warning? Just to tell the user how to proceed with datetime variables? :)

@jerneju
Copy link
Contributor Author

jerneju commented Mar 1, 2017

ISO 8601? Yes, program knows how to read "2017-03-01" but have no clue what means "2017-03-01T12:49:39+00:00" ("could not convert string to float").

@ajdapretnar
Copy link
Contributor

Complain to @kernc. It does work with 2017-03-01 12:49:39 though.
Can we come up with a good user message?

@jerneju
Copy link
Contributor Author

jerneju commented Mar 1, 2017

Great, I see it does work with that kind of format. I changed the code in a way that a user message won't be necessary. Now only variables which can be set to numerical can also be set to datetime.

Copy link
Member

@astaric astaric left a comment

Choose a reason for hiding this comment

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

While eventually we would prefer to allow user to specify the datetime format that would be able to parse the strings, this fix should prevent crashes in the mean time.

If possible, please add a test for the fix as well.

@@ -141,8 +141,11 @@ def setEditorData(self, combo, index):
no_numeric = not self.view.model().variables[
index.row()][Column.not_valid]
ind = self.items.index(index.data())
combo.addItems(self.items[:1] + self.items[1 + no_numeric:])
combo.setCurrentIndex(ind - (no_numeric and ind > 1))
combo.addItems(self.items[:1] +
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, this would be much clearer if written like:

if no_numeric:
  # Do not allow selection of numeric and datetime
  items = [i for i in self.items if i not in ("numeric", "datetime")]
else:
  items = self.items

ind = items.index(index.data())
combo.addItems(items)
combo.setCurrentIndex(ind)

ValueError  - could not convert string to float: '02.02.17'
https://sentry.io/biolab/orange3/issues/196743724/

Datetime option only if numerical.
@astaric
Copy link
Member

astaric commented Mar 3, 2017

Could you include a test?

@astaric astaric changed the title [FIX] ValueError owfile [FIX] File: Disallow changing string columns to datetime Mar 3, 2017
@lanzagar lanzagar self-assigned this Mar 3, 2017
@lanzagar lanzagar merged commit c3e2f7a into biolab:master Mar 3, 2017
@kernc
Copy link
Contributor

kernc commented Mar 3, 2017

From the top of the head, the reason it didn't work with:

2017-03-01T12:49:39+00:00

is that the colon in the timezone part doesn't parse with datetime.strptime. Standards-wise, I guess we deviate a little.

@jerneju jerneju deleted the valueerror-owfile branch April 20, 2017 13:47
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.

6 participants