-
Notifications
You must be signed in to change notification settings - Fork 19
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
mapping loading with mapping type specified or different method name #344
Conversation
b51c22e
to
a62c5ab
Compare
a62c5ab
to
04a8b2e
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #344 +/- ##
==========================================
+ Coverage 73.45% 73.87% +0.41%
==========================================
Files 91 92 +1
Lines 5831 5913 +82
==========================================
+ Hits 4283 4368 +85
+ Misses 1548 1545 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
1c4ca23
to
d3cad53
Compare
d3cad53
to
725eb93
Compare
29e9bf4
to
1789b65
Compare
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 good
Adresses #332
I guess the real issue is that a mapping may be a url, txt or a file path, and if it was neither a url (= couldn't request it) or a filepath (= couldn't load the file), then it was assumed to be a txt mapping. But it could have been intended to be a url but malformed, or a filepath but malformed.
Maybe some validation that whatever string is passed is meant to be a text mapping should be done. I've put a hack of a check that it should start with a {, but I'm not even sure this is true of all text mappings.
Update: in checking the validity of the mapping str provided, it brought out that many current mappings in tests are not valid paths, and the failure to load them was silent and did not make the tests fail