[No MRG, No Review] Refactor _fetch_file(s) #1835
Conversation
This reverts commit 1b22d18
* 'master' of https://github.com/nilearn/nilearn: Update nilearn/image/tests/test_resampling.py Update nilearn/image/tests/test_resampling.py Update nilearn/image/tests/test_resampling.py Updated News section of the website nilearn.github.io Updated number of commits by contributer Changed version from 0.5.0a to 0.5.0b Added list of contributors for the alpha release Updated whats new & aAdded list of contributors for the beta release Simplified the test and fixed an issue previously introduced in the doc. Renamed extrapolation parament "fill_value". Added one test. Added support for fill value in resampling tools.
* 'master' of https://github.com/nilearn/nilearn: Typos Proper fix fix css Clean doc build
| @@ -774,6 +765,49 @@ def _fetch_files(data_dir, files, resume=True, mock=False, verbose=1): | |||
| return files_ | |||
|
|
|||
|
|
|||
| def _download_or_mock_file(abort, dl_file, file_, mock, target_file, | |||
| temp_target_file, url): | |||
| if mock: | |||
There was a problem hiding this comment.
Could you add a one-line function description to explain in a nutshell what the function does ?
nilearn/datasets/utils.py
Outdated
| return abort | ||
|
|
||
|
|
||
| def _move_dataset_files(dl_file, opts, temp_dir): |
There was a problem hiding this comment.
(same for other private functions)
|
The appveyor failure is spurious. |
Codecov Report
@@ Coverage Diff @@
## master #1835 +/- ##
==========================================
+ Coverage 95.21% 95.21% +<.01%
==========================================
Files 135 135
Lines 17071 17092 +21
==========================================
+ Hits 16254 16275 +21
Misses 817 817
Continue to review full report at Codecov.
|
| target_file_does_not_exist = not os.path.exists(file_1) | ||
| temp_target_file_does_not_exist = not os.path.exists(file_2) | ||
| file_does_not_exist = target_file_does_not_exist and temp_target_file_does_not_exist | ||
| return file_does_not_exist |
There was a problem hiding this comment.
3-liner functions are in general bad practice. The reason being that when doing a careful read of the codebase, one needs to read them to know what they actually do (the name is not a contract, and bugs hide this).
I am not very enthousiastic about merging changes that turn 3 lines of code used twice into a function. It makes the code harder to read and maintain.
| os.remove(file_) | ||
| except Exception as e: | ||
| abort = str(e) | ||
| return abort |
There was a problem hiding this comment.
This function is used only once, and understanding what it does is as difficult from the code as from the function name and description. It should not be factored out.
| ------- | ||
| dst_filepath: string | ||
| Path to the moved file. | ||
| """ |
There was a problem hiding this comment.
Another function that is very simple and used once.
|
@kchawla-pi : I am really sorry, but these changes do not make the codebase easier to maintain, rather harder. I've worked on many codebases, and codebases that are structured like this (with many tiny functions used once or twice) are very hard to follow: they require jumping from one function to another all the time, and keeping in one's working memory what the functions do. I am not in favor of merging these changes. Let's try to discuss this IRL. |
kchawla-pi
changed the title
|
I am open to learning a better way to refactor existing code to improve its readability. |
While working on #1792 I found it painful & slow to read the _fetch_files() & fetch_file().
Additionally, the new URLs on OSF may necessitate changing how the passed in URLs are handled.
As per issue #1769, I have refactored the function into several smaller ones, which have made it considerably easier for me to comprehend it, and modify it if necessary.
The logic has not been altered to maintain Python2 compatibility & make the refactoring quick, safe, and easy.
This is in line with industry standard clean code principles for improving code quality and something I intend to do simultaneously where ever I see code that is difficult to comprehend.