style: fix flake8 error for tools.py #9
Conversation
|
@sbillinge ready for review. I'll fix the pre-commit errors first before skpkging this! |
src/diffpy/srxconfutils/tools.py
Outdated
| :return: crc32 value of file | ||
| """ | ||
| try: | ||
| fd = open(filename, "rb") |
There was a problem hiding this comment.
Its probably a good idea to bring this all up to our general standards, not just flake8 (e.g., in this case, using context manager for file reads). On the minus side, it makes these flinting corrections go more slowly but on the plus side, these are hard to find later and fix if we don't fix them when we find them. An alternative is to put a # fixme and then have an issue that later goes back and cleans the fixme's
src/diffpy/srxconfutils/tools.py
Outdated
| try: | ||
| fd = open(filename, "rb") | ||
| except: | ||
| except Exception: |
There was a problem hiding this comment.
I would prefer a more precise exception here, and maybe a more robust way of handling it? Returning some random string? What does other code that call this function do with this string?
|
@sbillinge ready for review |
sbillinge
left a comment
sbillinge
left a comment
There was a problem hiding this comment.
This looks good. Let's take the opportunity to write a test for this. Also make the function name pep8 compliant (lower-case)
|
@sbillinge ready for review |
sbillinge
left a comment
sbillinge
left a comment
There was a problem hiding this comment.
please see comments. I think if we just move the tmpfile to conftest then I can merge.
tests/test_tools.py
Outdated
| from diffpy.srxconfutils import tools | ||
|
|
||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
arguably we might want to put this in conftest.py so it is available to all tests, or something like it that can be extended in each test.
tests/test_tools.py
Outdated
| return file_path, content | ||
|
|
||
|
|
||
| def test_check_md5(temp_file): |
There was a problem hiding this comment.
I would guess that a function called "check_md5would return True or False. If it returns an md5, then maybeget_md5`? but since doing that is one line why do we need a function? Is it that it takes a path and then finds a file and returns its hash?
We could handle all this a bit better but it is probably not worth spending the time at this point. Thanks for writing this test at least. I will merge it like this.
There was a problem hiding this comment.
You're right, I believe that get_md5 would make more sense for the function name. From what I understand, the purpose of the function is that it handles reading large files in chunks and wraps errors nicely. I will also rename the check_crc32 function`.
|
@sbillinge ready for review |
|
super, this looks great, I merged. btw, did you do the test that this didn't break anything? If, as you mention, these tools are used to help with file chunking, it is probably worth spending a few minutes to see if there are packages for this, as this is a very standard task done by everyone. For example, since we are using gcp, we could use tools in |
|
Yes, I did do the test to make sure that this didn't break anything. However, I'm not very familiar with gcp or the google-cloud-storage package. Do you want me to look into these and find out how I can implement them into our code? |
|
Actually, I got myselft confused @zmx27 , this code doesn't interact with the internet perse. Do we know why it is chunking files? |
2 participants
No description provided.