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 reader.py:read_file_contents - unused #393

Merged
merged 1 commit into from
Jan 19, 2021
Merged

Conversation

kinverarity1
Copy link
Owner

I think as part of #327 I was probably meant to remove the function reader.py:read_file_contents. All tests pass without this function, however coverage is not 100% so I want to be sure before removing the code.

@dcslagel you have probably looked more closely over the related changes than I did; does this seem right to you?

@dcslagel
Copy link
Collaborator

dcslagel commented Sep 14, 2020

@kinverarity1, I have a caution about removing read_file_contents() too quickly also.

I did the following searches that show Lasio is no-longer calling read_file_contents.

$ cd lasio
$ grep -rl read_file_contents *
docs/source/lasio.rst
htmlcov/lasio_las_py.html
htmlcov/lasio_debug-reader_py.html
htmlcov/lasio_reader_py.html
lasio/reader.py

$ grep  read_file_contents lasio/reader.py
def read_file_contents(

However, In scanning the removing read_file_contents() code, there are some interesting details we should check whether if they are relevant and if so, exist in the new code.

Could this pull-request be kept around for a while to give us time to read through the code through read through?

Thanks,
DC

@kinverarity1
Copy link
Owner Author

Yep no hurry 👍

@dcslagel
Copy link
Collaborator

dcslagel commented Sep 15, 2020

@kinverarity1, I reviewed reader:read_file_contents() and compared with the current code. It is okay to proceed with removing reader:read_file_contents().

Findings:

@kinverarity1
Copy link
Owner Author

Thanks @dcslagel

@kinverarity1 kinverarity1 merged commit 73568c8 into master Jan 19, 2021
@kinverarity1 kinverarity1 deleted the remove-old-code branch January 19, 2021 11:03
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.

None yet

2 participants