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

Remove utils.move_file() function as it is no longer used in the toolbox #342

Merged
merged 4 commits into from
Nov 11, 2020

Conversation

viacovella
Copy link
Contributor

@viacovella viacovella commented Nov 11, 2020

#Closes #336 and #closes #337

Proposed Changes

  • Removes the in-house built function to move the files from the Utilities collection;
  • Removes the test for in-house built function to move the files from the Unit test for Utilities collection.

@eurunuela eurunuela changed the title Issue #336 Replacing Utils.move_file with Shutil.move function Removed utils.move_file function as it is no longer used in the toolbox Nov 11, 2020
Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

LGTM!

@viacovella can you change the first line in the first PR message to "Closes #336 and closes #337"?

@smoia
Copy link
Member

smoia commented Nov 11, 2020

Oh, I spoke too soon 😅

The style test is failing because there was a line missing between two functions. Can you correct that please?

@smoia smoia added the Refactoring Improve nonfunctional attributes label Nov 11, 2020
@eurunuela
Copy link
Collaborator

Thank you @viacovella ! I think this is a very simple refactoring PR and two reviews are not really necessary for this one. What do you think @smoia ?

Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

Ok, we're close @viacovella, we're close.

@smoia
Copy link
Member

smoia commented Nov 11, 2020

@viacovella flake8 is not your friend today!

Check here to know what the problem is.

In this case, apparently there's a whitespace in the line you added.

Can I ask you what are you using to write your scripts? Sometimes adding some linter plugin helps dramatically in avoiding these little issues. I can help you set them up.

@eurunuela
Copy link
Collaborator

Can I ask you what are you using to write your scripts? Sometimes adding some linter plugin helps dramatically in avoiding these little issues. I can help you wet them up.

I'd also suggest that you tune your IDE settings to show you whitespace. It's incredibly useful for these kinds of errors.

Copy link
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

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

The style check passed, so this should be ready.

Thank you @viacovella !

@codecov
Copy link

codecov bot commented Nov 11, 2020

Codecov Report

Merging #342 (0d8cbb9) into master (106f8ad) will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
+ Coverage   94.81%   94.89%   +0.08%     
==========================================
  Files           9        9              
  Lines         848      843       -5     
==========================================
- Hits          804      800       -4     
+ Misses         44       43       -1     
Impacted Files Coverage Δ
phys2bids/utils.py 98.59% <ø> (+1.22%) ⬆️

@smoia
Copy link
Member

smoia commented Nov 11, 2020

Ok, I think this is ready!

Since this is not only your first PR, but your first Brainhack PR, here's a gong for you!

🛎️

(Or the closest thing we can get with GH emojis)

@smoia smoia changed the title Removed utils.move_file function as it is no longer used in the toolbox Remove utils.move_file function as it is no longer used in the toolbox Nov 11, 2020
@smoia smoia merged commit a6d6085 into physiopy:master Nov 11, 2020
@eurunuela
Copy link
Collaborator

This PR won't trigger auto right?

@smoia
Copy link
Member

smoia commented Nov 11, 2020

I have no idea since it's the first refactoring we merge. I don't think so though, no.

@smoia smoia changed the title Remove utils.move_file function as it is no longer used in the toolbox Remove utils.move_file() function as it is no longer used in the toolbox Nov 13, 2020
@smoia
Copy link
Member

smoia commented Nov 29, 2020

🚀 PR was released in 2.3.0 🚀

@smoia smoia added the released This issue/pull request has been released. label Nov 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring Improve nonfunctional attributes released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace utils.move_file() with shutil.move()
3 participants