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

[ENH] Normalize all the line endings #191

Merged
merged 1 commit into from
Feb 6, 2019
Merged

[ENH] Normalize all the line endings #191

merged 1 commit into from
Feb 6, 2019

Conversation

jbteves
Copy link
Collaborator

@jbteves jbteves commented Jan 21, 2019

Closes #190 .

Changes proposed in this pull request:

  • normalizes line endings for Windows compatibility

Note: tested on MacOSX and Windows for cross-compatibility.

@codecov
Copy link

codecov bot commented Jan 22, 2019

Codecov Report

Merging #191 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #191   +/-   ##
=======================================
  Coverage   51.58%   51.58%           
=======================================
  Files          32       32           
  Lines        1958     1958           
=======================================
  Hits         1010     1010           
  Misses        948      948

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 0b4e0e3...e39223b. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jan 22, 2019

Codecov Report

Merging #191 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #191   +/-   ##
=======================================
  Coverage   51.58%   51.58%           
=======================================
  Files          32       32           
  Lines        1958     1958           
=======================================
  Hits         1010     1010           
  Misses        948      948

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 0b4e0e3...e39223b. Read the comment docs.

@emdupre
Copy link
Member

emdupre commented Jan 22, 2019

Thanks, @jbteves ! It looks like this is modifying the binary nifti files, though ? Was that intentional ?

@jbteves
Copy link
Collaborator Author

jbteves commented Jan 22, 2019

No, it's just stating that any .nii files should be handled like binary files-- regardless of platform, the contents should be precisely the same.

@jbteves
Copy link
Collaborator Author

jbteves commented Jan 22, 2019

I see now-- I'm not sure what that mode change means, but according to git the actual contents of the files shouldn't be different. Maybe we can check by loading up the contents on two systems and subtracting the images?

@jbteves jbteves changed the title Normalize all the line endings [ENH] Normalize all the line endings Jan 23, 2019
@jbteves
Copy link
Collaborator Author

jbteves commented Jan 24, 2019

@emdupre I researched the problem a bit more. Apparently the directory structure retains file permissions data for each file. So the files themselves are not modified, but their permissions are. We can either revert that part of the change or we can add

core.filemode false

to the git config. More info here. I'm not sure why but the issue appears to only be introduced if you clone into Windows.

@emdupre
Copy link
Member

emdupre commented Feb 1, 2019

OK, now that #189 is merged I'd like to tackle this one !

So it sounds like the only remaining question is whether we should update the permissions on the nifti files. Could you expand on the benefit to having those permissions updated ?

@jbteves
Copy link
Collaborator Author

jbteves commented Feb 1, 2019

Hm, so I've simmered on this a bit. I'm not sure if tedana formally offers Windows support-- if not then there's no reason to introduce these changes. As far as permissions go, all it does is make the files non-executable. I assume that this is okay because there's no reason to execute a .nii file, and view that as an improvement. For some reason on my Windows machine this permission is coerced-- it may be because I don't have a default .nii reader available and the OS is protecting me.

@emdupre
Copy link
Member

emdupre commented Feb 1, 2019

We only require a python environment, at this point, so we should be cross-platform. I'm not sure why Windows would require that permissions update, but I just wanted to be sure it was necessary for our n=1 Windows sample 😄

I'm ok to merge this, then. @tsalo @rmarkello, please voice if you have any objection !

@jbteves
Copy link
Collaborator Author

jbteves commented Feb 1, 2019

Hm, let me amend the commit message to include the file mode changes. Otherwise on review that might be confusing.

@jbteves
Copy link
Collaborator Author

jbteves commented Feb 1, 2019

Scratch that, I forgot that amending commits that are already referenced remotely is typically bad practice-- we can just include this and read the context discussion out of the PR. Should be easy enough since it's just the one.

@emdupre
Copy link
Member

emdupre commented Feb 6, 2019

I'm going to go ahead and merge this so we can cut our next release. If it causes an issue in the wild, we'll issue a hotfix (but I think it should be ok !). Thanks, @jbteves

@emdupre emdupre merged commit a22a7c2 into ME-ICA:master Feb 6, 2019
@jbteves jbteves deleted the JT_Fix_LineEndings branch May 2, 2019 22:04
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.

Cross-Platform Line Endings
2 participants