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 bug where floats were saved as strings #114

Merged
merged 1 commit into from
Nov 3, 2017

Conversation

nickfong
Copy link
Contributor

When values in the PropertyEditor were changed by typing in new values, new values were saved as strings, which caused a crash when opening the saved file.

This commit casts values of keys that should be floats to floats. Looked into UI-side validation (since right now the editor allows non-floats to be entered as a value) without much success; it seems that a change in PropertyEditorWidget.py (and associated classes) would need to be made.

Fixes #112

When values in the PropertyEditor were changed by typing in new values,
new values were saved as strings, which caused a crash when opening the
saved file.

This commit casts values of keys that should be floats to floats.
Looked into UI-side validation (since right now the editor allows
non-floats to be entered as a value) without much success; it seems that
a change in PropertyEditorWidget.py (and associated classes) would need
to be made.

Fixes cadnano#112
@nickfong nickfong added the bug label Oct 23, 2017
@nickfong nickfong requested a review from sdouglas October 23, 2017 18:30
@sdouglas sdouglas merged commit 9e2356c into cadnano:master Nov 3, 2017
@scholer
Copy link
Contributor

scholer commented Dec 21, 2017

This PR introduces a bug in line 1278 where it refers to basestring in the check for string type. The basestring symbol/class (from which str and unicode was derived in Python 2) is not available in Python 3+, and python will throw a NameError. Instead of basestring, we should just use str.

Do you want a separate PR for changing basestring → str?

@sdouglas
Copy link
Contributor

Thanks, @scholer - we noticed and fixed this elsewhere but didn't merge the change into master. @nickfong and I will resolve it next week.

@nickfong nickfong deleted the bugfix/issue-112 branch February 27, 2018 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants