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

Change "is" check for 'GCV' #1769

Merged
merged 1 commit into from Mar 15, 2019
Merged

Change "is" check for 'GCV' #1769

merged 1 commit into from Mar 15, 2019

Conversation

mattcieslak
Copy link
Contributor

This is causing an error in python 3.7

This is causing an error in python 3.7
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@78f74e8). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1769   +/-   ##
=========================================
  Coverage          ?   83.88%           
=========================================
  Files             ?      120           
  Lines             ?    14572           
  Branches          ?     2295           
=========================================
  Hits              ?    12224           
  Misses            ?     1824           
  Partials          ?      524
Impacted Files Coverage Δ
dipy/reconst/mapmri.py 90.33% <100%> (ø)

@arokem
Copy link
Contributor

arokem commented Mar 15, 2019

Hey @mattcieslak - thanks for fixing this. Under what circumstances do you see this raising an error in Python 3.7? Is this something not currently covered by tests?

@mattcieslak
Copy link
Contributor Author

It works in tests, but the string that ends up being compared in my code comes from a traits.Str object (https://github.com/PennBBL/qsiprep/blob/master/qsiprep/interfaces/dipy.py#L216), so checking the address using is doesn't work. Using == will convert it to a string and works.

@arokem
Copy link
Contributor

arokem commented Mar 15, 2019

OK. I figure it's a small change and since it sounds like testing it would be more than we need, I'm just going to go ahead and merge this.

@arokem arokem merged commit bfde017 into dipy:master Mar 15, 2019
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.

None yet

3 participants