feat: switch User and Content to inherit dict, add update method for User

[nealrichardson]

To help us move the discussion forward. I switched User to inherit from dict to see how that works, including adding an update method and wiring that up. I first tried UserDict but it messed up the sort order of the columns in the DataFrame version, and googling a bit, it sounded like it wasn't so bad to inherit from dict anymore, so I figured I'd try that and see how it went. Summary of what I found so far, as reflected in the test suite:

• ✅ Forwards and backwards compatibility (i.e., what happens if the API returns more or fewer fields than the class definition knows about) all works fine. If new fields appear and you haven't defined propertys for them, it doesn't break, and you can still get at them via [. Missing fields also are fine, at least as long as you don't access them (you'd get a KeyError).
• ✅ update() method works. I was worried from that blog post that you wouldn't be able to define a new one, but it was fine. I did not try setattr, and my guess is that we'd want to disable that anyway (but that's a discussion for another time).
• 🤔 pd.DataFrame(list of Users) works fine since it is a dict still, but: in adding the session and url to the object in order to implement update(), those now show up in the data frame. AFAIK there's no way to hide them somewhere else, it's just a dict.
• 🤔 @propertys work to access the data, but the type hints don't enforce anything. I didn't add in the datetime parsing, just to see what happens, and the methods happily return the unparsed strings. Not strictly a problem, but worth keeping in mind.
• Other consideration related to the last two: we could add an __init__ method to do things like parse timestamps. As we discussed before, there are tradeoffs to doing this eagerly vs. on demand. Alternatively, if we kept it lazy (my preference), we could add a to_pandas function or something that would parse them when you do that conversion, and leave the attribute parsing to the property getters.

[github-actions]

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
487	391	80%	80%	🟢

New Files

No new covered files...

Modified Files

File	Coverage	Status
src/posit/connect/content.py	72%	🟢
src/posit/connect/users.py	83%	🟢
TOTAL	78%	🟢

updated for commit: f7c606a by action🐍

[nealrichardson]

I've updated both User and ContentItem to inherit from dict and added all of the property accessors. I also revised the update() method for User to enumerate (and type) the fields you can update. I think I should stop here and make more explicit followups for the next steps (various TODOs left in the code, for one), rather than roll more into this PR.

One issue I could use help with: mypy does not like this change.

[nealrichardson]

Ok @tdstein I think this is done (enough) and we should merge so we can unblock other work.

[tdstein]

🤔 pd.DataFrame(list of Users) works fine since it is a dict still, but: in adding the session and url to the object in order to implement update(), those now show up in the data frame. AFAIK there's no way to hide them somewhere else, it's just a dict.

Can we implement an __init__ method in Resource as follows? I haven't tried it yet.

def __init__(self, session: requests.Session, url: str, **kwargs):
    self._session = session
    self._url = url
    super().__init__(**kwargs)

[tdstein]

Other consideration related to the last two: we could add an init method to do things like parse timestamps. As we discussed before, there are tradeoffs to doing this eagerly vs. on demand. Alternatively, if we kept it lazy (my preference), we could add a to_pandas function or something that would parse them when you do that conversion, and leave the attribute parsing to the property getters.

Let's go ahead and defer this decision until later. I'm satisfied that pandas.DataFrame(users) works. 😄

[tdstein]

This looks good. But should we enforce these logical constraints or defer this behavior to the server? In other words, how do we handle behavioral changes to the PATCH endpoint between versions of Connect?

[nealrichardson]

Suppose a future version of Connect adds a new field you can PATCH to update. We would add it here explicitly, and users would need to upgrade the SDK to the latest to access it. That seems fine.

Suppose a future version of Connect changed it so that user_role didn't exist, and instead you provided a permissions dict of named booleans. We would add a check here for the version of the Connect server, and if you provided user_role to a version of Connect that only supported permissions, map our understanding of our current roles to those permissions bundles (or error). We could also do the reverse and map known permission bundles back to user_role (or error).

So I think this is ok, or at least we have options.

[tdstein]

Ok @tdstein I think this is done (enough) and we should merge so we can unblock other work.

Agreed. Let's merge and continue iterating.

[nealrichardson]

🤔 pd.DataFrame(list of Users) works fine since it is a dict still, but: in adding the session and url to the object in order to implement update(), those now show up in the data frame. AFAIK there's no way to hide them somewhere else, it's just a dict.

Can we implement an __init__ method in Resource as follows? I haven't tried it yet.

def __init__(self, session: requests.Session, url: str, **kwargs):
    self._session = session
    self._url = url
    super().__init__(**kwargs)

I don't think that will succeed in hiding the session and url from the dict itself since it inherits dict, but we can experiment more. Let's merge this and then your other PR, and then we can tinker with the base class.