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

Resolved two issues: added support for MKV files and clarified "error" printed output #461

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

D3Zyre
Copy link

@D3Zyre D3Zyre commented Oct 13, 2023

resolved: #457
resolved: #460

there is now a new type of result, duplicate files
these are files that already exist in the destination
folder and are not imported if not importing duplicates

I have thoroughly verified that my changes do not cause
any other part of the code to break or act in an
unexpected manner. I have also removed some code
that could never get used:
elodie/localstorage.py: checksum() could never return None,
code was not accessible due to earlier return
because of this:
elodie/filesystem.py: process_checksum() did not need
to check if checksum was None after calculating it
this is what allowed my new code to use None
as a flag meaning that the file being imported and skipped
is a duplicate file
@CLAassistant
Copy link

CLAassistant commented Oct 13, 2023

CLA assistant check
All committers have signed the CLA.

@D3Zyre
Copy link
Author

D3Zyre commented Oct 13, 2023

The fail on ci/circleci is due to the printed output being changed in result.py. This is expected behavior for this PR (Issue #460)
the solution to #460 was to clarify in the printed output when files are not imported due to being duplicates of already imported files, instead of referring to them as "errors"

@D3Zyre
Copy link
Author

D3Zyre commented Oct 13, 2023

I am working on updating the tests for my code, I'll need to create another PR when it's done.

I have noticed that my code does fail 4 tests, even after updating the tests. so I will figure that out now

@coveralls
Copy link

coveralls commented Oct 13, 2023

Coverage Status

coverage: 90.884% (+0.3%) from 90.61% when pulling 6e946d6 on D3Zyre:main into 76ad823 on jmathai:master.

@D3Zyre
Copy link
Author

D3Zyre commented Oct 13, 2023

oh I don't need to make a new PR apparently. cool.

Well as you can see I've updated the tests to the new formatting and fixed the minor issue that had caused 4 of them to fail

@D3Zyre
Copy link
Author

D3Zyre commented Oct 13, 2023

On this note:

We should really add some test cases to test the functionality of trying to import a file that has already been imported, to see if the duplicate count goes up

@D3Zyre
Copy link
Author

D3Zyre commented Oct 13, 2023

the test that has now failed (for whatever reason) has nothing to do with any code I have changed. Odd

@D3Zyre
Copy link
Author

D3Zyre commented Oct 13, 2023

I changed nothing and now the test passes. It seems that the failed test was a HTTPS call to that geolocation server. Annoying

@D3Zyre
Copy link
Author

D3Zyre commented Oct 13, 2023

Another note. Is there any documentation that needs to be updated with this "UI" change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants