canvasapi icon indicating copy to clipboard operation
canvasapi copied to clipboard

Fixed file objects with hyphen issue (ucfopen#647 ) and added test cases

Open nayanpaa opened this issue 4 months ago • 7 comments

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%

nayanpaa avatar Apr 23 '24 02:04 nayanpaa

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.)

dbosk avatar Apr 23 '24 04:04 dbosk

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.

bennettscience avatar Apr 23 '24 10:04 bennettscience

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"?

nayanpaa avatar Apr 23 '24 23:04 nayanpaa

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.

bennettscience avatar Apr 24 '24 00:04 bennettscience

Build failed, I think my code didn't follow the contribution guidelines. Just committed the reformatted code.

nayanpaa avatar Apr 24 '24 23:04 nayanpaa

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

jonespm avatar Apr 30 '24 13:04 jonespm

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.

bennettscience avatar Apr 30 '24 16:04 bennettscience