-
Notifications
You must be signed in to change notification settings - Fork 77
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
bug fix for the replacement of dl1 suffix #413
Conversation
Codecov Report
@@ Coverage Diff @@
## master #413 +/- ##
==========================================
+ Coverage 41.74% 41.76% +0.01%
==========================================
Files 77 77
Lines 6211 6213 +2
==========================================
+ Hits 2593 2595 +2
Misses 3618 3618
Continue to review full report at Codecov.
|
Hi @SeiyaNozaki. I believe this may have been intended, take into account that the first file is a real data zfits file and the second one is a MC one and |
@rlopezcoto no, this is clearly a bug. Only the suffixes defined in So I would only suggest adding a test case for the file that was not correctly converted before @SeiyaNozaki |
If file name includes dots before suffixes, the result is always bad.
So for the above case both of the results are bad(".Run01817." in real data, "0.4" in MC) |
Yes, sure. You're fix is correct. Could you add one example where the current code breaks to the tests? |
@maxnoe ok, I added the examples, but I got the quality review error(Use of assert detected)... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got the quality review error(Use of assert detected)...How should I proceed?
@SeiyaNozaki it is alright
This codacy check unfortunately is not really reliable. Use of assert is required for unit tests, so it's ok. (Codacy should know that!) |
When using
r0_to_dl1_filename
, it seems another segment is removed by usingwith_suffix
.Here I just replaced
with_suffix
with+
.