canvasapi
canvasapi copied to clipboard
Fixed file objects with hyphen issue (ucfopen#647 ) and added test cases
Proposed Changes
- Based off the description of the issue in #647, the issue of attributes with hyphens can be narrowed down to set_attributes in canvas_object.py. Any attribute in the JSOn object with a hyphen("-") is translated to an underscore("_").
- In test_canvas_object.py, I have added a function to include test cases that may target this issue.
Effects
- With these changes, all tests are passing, and the coverage report remains at 100%
One thought, maybe we should keep the old behavior too? So that both works. Because, while being a bug, this would still be a breaking change. (I don't like the redundancy that is the result of that though. So I prefer it the way it is now.)
I think I agree with @dbosk about keeping both in there for now. One solution would be to throw a warning into the function that will display when files are downloaded. DeprecationWarning
is suppressed by default, so UserWarning
might be the best course. Here's how warnings are used in the Canvas
module:
if "http://" in base_url:
warnings.warn(
"Canvas may respond unexpectedly when making requests to HTTP "
"URLs. If possible, please use HTTPS.",
UserWarning,
)
Don't forget to import warnings
at the top of the module.
I see, I think when I approached this issue I assumed that hypens were common in many attributes. So now I will only translate hyphens to underscores in the case that I get the attribute "content-type"?
As far as I know, that's the only attribute returned specifically from Canvas with a hyphen in the name. I don't have any guess as to how many people are operating on files, but this would be a tricky bug to sort out without any sort of lead time. Keeping the old content-type
attribute with a warning pointing people to update to content_type
will prevent major headaches. We can also mark the hyphenated attribute to be removed in a future version.
Build failed, I think my code didn't follow the contribution guidelines. Just committed the reformatted code.
I modified this to be more generic by internally storing these as dict so anything that has a dash or not a dash in i works. Both values dashed and not dashed are accessible. This passes all of the existing tests but I removed the test for the DeprecationWarning since I'd removed it. Can have this as a followup or just addon to this issue. There was a special case in this code where validation_token still needed to actually be an actual attribute so I just left that as-is.
https://github.com/ucfopen/canvasapi/commit/78620a0b935bf463a647a044af69328af0db9417
That change makes sense to me...I had to do a bunch of reading to understand the difference between __getattr__
and __getattribute__
, so I appreciate that this handles either case. It's also more robust if Instructure decides to do weird things with models down the road like they've done with quiz
and new_quiz
objects.