Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0383237
fixed issue 647 and added test cases
nayanpaa Apr 23, 2024
b087966
changed to keep both attributes and added corresponding tests
nayanpaa Apr 24, 2024
ae6c387
now following correct style implementations for issue #647
nayanpaa Apr 24, 2024
5c02d7e
Fix isort
Thetwam Apr 29, 2024
51e59d1
Add failing test to demonstrate issue with content_type conversion
Thetwam Apr 29, 2024
66680c6
Add failing test to check for DeprecationWarning when accessing conte…
Thetwam Apr 29, 2024
583e9c3
Moved content-type attribute deprecation warning to getter
Thetwam Apr 29, 2024
fd067da
Merge branch 'develop' into issue/647-File-objects-get-attribute-cont…
Thetwam May 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions canvasapi/canvas_object.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import arrow
import pytz
import warnings


class CanvasObject(object):
Expand Down Expand Up @@ -60,6 +61,16 @@ def set_attributes(self, attributes):
"""
for attribute, value in attributes.items():
self.__setattr__(attribute, value)
if attribute == "content-type":
self.__setattr__("content_type", value)
Comment on lines +74 to +75
Copy link
Member

@Thetwam Thetwam Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the problem here is with content-type, it technically happens with any value that contains a -. I'm not aware of any other similar values in Canvas' responses, but in theory it's possible. Whatever solution we decide on for content-type could be generalized by replacing all - with _.

If there are major concerns with a broader approach, I'm happy to adopt a content-only fix for now and spin it off into its own issue for later.

Copy link
Contributor

@bennettscience bennettscience Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a cursory glance, I couldn't see any other response attributes that had hyphens. There are some other endpoints where camelCase is used (like the result and score endpoints), but those are only to report to IMS and LTI tools.

I think the question for me is which is more Pythonic - changing all the returned properties into class attributes or keeping the object as close to the returned structure (ie, keeping a hyphenated name and requiring the getattr method) from Canvas so the library matches the docs?

Copy link
Contributor

@jonespm jonespm Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also thinking of making it more generic, like if the value contained a hyphen to set it as a value with an underscore. I think I'm fine for making this into a future issue.

It really might be easiest to change this class to store all of the variables in a dictionary. Then the __getattribute__ and __getattr__ can be used to return from this dictionary by default. Then we can store both forms of the value with and without underscores. I'm fine with that as a separate issue but seems like it might be harder to get that fixed.

Copy link

@dbosk dbosk May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original PR proposed the general replace-all-hyphens-with-underscores. I also favour that solution. Canvas might add new attributes that have hyphens instead of underscores.

I also think it makes the most sense to keep the hyphenated version and provide the underscored version for convenience, to match the documentation as mentioned above. But it's important to get it right so that they sync, in case they're changed (although it doesn't make sense to change this particular attribute).

Edit: Looked at @jonespm's solution below, I also think that's the way to do it.

warnings.warn(
(
"The 'content-type' attribute will be removed "
"in a future version. Please use "
"'content_type' instead."
),
UserWarning,
)
Copy link
Member

@Thetwam Thetwam Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate what @bennettscience was suggesting here, but unfortunately the user doesn't typically have control over what gets passed into this function. Canvas is the one who sends the content-type attributes back, and we have to accept them as-is. Adding this will result in a bunch of warnings that the user will have no recourse to fix. We also wouldn't be able to remove the attribute in a future version until Canvas does.

Copy link
Member

@Thetwam Thetwam Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon re-reading the discussion, was the intention to warn users who were trying to access content-type via getattr? In that case, a warning could make sense but it would have to be on the getter function, not the setter function as it is here.

I think modifying __getattribute__ could do the trick?

Copy link
Contributor

@jonespm jonespm Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Thetwam I just looekd at this too and was thinking the same, that it's probably better to have this in a __getattribute__ or possibly even better in __getattr__ rather than in the set_attributes. Then you don't lose a similarly named attribute with.

__getattr__ would only catch the ones that don't exist so wouldn't need to run through for everything.

I think it's unlikely they'd have a content_type and content-type but it's possible for other headers.

Copy link
Member

@Thetwam Thetwam Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL there's difference between __getattribute__ and __getattr__.

However, if I'm understanding it correctly, we'd still want to use __getattribute__ for the time being, since the current behavior is to keep content-type while adding content_type.

I moved the warning to __getattribute__, which passes the test__getattribute__content_type_warns test, whereas __getattr__ fails (doesn't throw the expected warning)

Copy link
Contributor

@jonespm jonespm Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, my idea wouldn't work as it is setting the values right on the class rather than as an dictionary in the class. This seems good to me.


try:
naive = arrow.get(str(value)).datetime
Expand Down
17 changes: 17 additions & 0 deletions tests/test_canvas_object.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,20 @@ def test_set_attributes_invalid_date(self, m):
self.assertFalse(hasattr(self.canvas_object, "end_at_date"))
self.assertTrue(hasattr(self.canvas_object, "start_at"))
self.assertTrue(hasattr(self.canvas_object, "end_at"))

# set_attributes 'content-type'
def test_set_attributes_with_content_type(self, m):
attributes = {
"content-type": "application/json",
"content_type": "another_application/json",
"filename": "example.json",
}

self.canvas_object.set_attributes(attributes)

self.assertTrue(hasattr(self.canvas_object, "content-type"))
self.assertEqual(getattr(self.canvas_object, 'content-type'), "application/json")
self.assertTrue(hasattr(self.canvas_object, "content_type"))
self.assertEqual(self.canvas_object.content_type, "another_application/json")
Copy link
Member

@Thetwam Thetwam Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test brings up some bigger questions about how we want to handle the case where both the - and _ versions of what is otherwise the same variable are set. In this case, the order of the items in the dictionary matters. I'll create and push new a test case the same as this one but reversing the order to demonstrate.

It's pretty unlikely that Canvas would send back two attributes with only the -/_ differentiating them, let alone with different values, but it's something we'll need to consider how we want to handle.

Copy link
Contributor

@jonespm jonespm Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought about this too, I think we can solve both of those problems but maybe in a separate issue. I don't think it's super likely either.

Copy link

@dbosk dbosk May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth adding tests that detect if Canvas would ever do this?

I do quite some API "reverse engineering" of APIs I want to use but don't have any official documentation (they're for "internal" use of the web UI). Then I add tests that will fail whenever some return value from the server changes.

Such tests must be added as a separate test suite run for this particular purpose. It would require a Canvas instance to run them against and, consequently, a login and token to the API. So not the ordinary test setup.

I'm leaning towards not worth it. Canvas is pretty stable and documents everything. So the likelihood of being useful is close to zero.

self.assertTrue(hasattr(self.canvas_object, "filename"))
self.assertEqual(self.canvas_object.filename, "example.json")