-
Notifications
You must be signed in to change notification settings - Fork 11
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
Avoid fail when file does not exist #175
Conversation
Codecov Report
@@ Coverage Diff @@
## master #175 +/- ##
==========================================
- Coverage 76.52% 76.37% -0.16%
==========================================
Files 19 19
Lines 3766 3775 +9
Branches 471 474 +3
==========================================
+ Hits 2882 2883 +1
- Misses 643 650 +7
- Partials 241 242 +1
Continue to review full report at Codecov.
|
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.
Looks pretty good! One comment on the error message, and also one question: do you want to update the tests at all to hit these new branches? Now that we have Sentry, and this is already an exception path, I don't think it's a blocker, but figured I'd ask.
else: | ||
warnings.append( | ||
'file "%s" does not exist, please check that its name is spelled correctly, ' | ||
'that it has not been renamed or removed' % (filename,) |
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.
This is a good, actionable error message. However, we should keep an eye out for if there are many, many missing files. Assuming there are 0-10, I think the error display method defined in the JS above should be fine.
@mahmoud Yes, I think we should add some tests for situations like that (there are also several situations around that should be handled in a similar way). BTW, have you checked my previous test improvements #159? |
Cool. So, what I'm understanding is that we can roll with this for now and will improve error display in a future PR? As for #159, somehow I missed that! Left a comment. :) |
@baturin just checking in on this, are we OK to merge + address errors in the next? |
@mahmoud Yeah, let's continue improving error reporting in future PRs, and merge that one. |
Let's start sorting out issues from #141. That simple PR avoids montage from failure when importing renamed/removed/incorrect image files.