Item models

[drio18]

Here, we add "item models" to snovault, specifically adding the ability to navigate linkTos with requests.

Related dcicutils PR here: 4dn-dcic/utils#269.

[netsettler]

My comments here are primarily syntactic. There are a couple of semantic questions I need an answer to in order to understand this, though, so maybe you can work through this and then we can re-discuss it after. If you disagree with my syntactic suggestions, please contact me on slack so we don't have to have an extended discussion about that on the PR itself. It will go faster another way. :)

[netsettler]

Suggested change

	def from_properties(
	cls,
	properties: Dict[str, Any],
	fetch_links=False,
	auth=None,
	request=None,
	**kwargs: Any,
	) -> PortalItem:
	def from_properties(cls, properties: Dict[str, Any], fetch_links=False, auth=None, request=None,
	**kwargs: Any) -> PortalItem:

[netsettler]

Suggested change

	def from_identifier_and_existing_item(
	cls, identifier: str, existing_item: PortalItem, **kwargs: Any
	) -> PortalItem:
	def from_identifier_and_existing_item(cls, identifier: str, existing_item: PortalItem, **kwargs: Any) -> PortalItem:

[netsettler]

Suggested change

	def from_properties_and_existing_item(
	cls, properties: Dict[str, Any], existing_item: PortalItem, **kwargs: Any
	) -> PortalItem:
	def from_properties_and_existing_item(cls, properties: Dict[str, Any], existing_item: PortalItem,
	**kwargs: Any) -> PortalItem:

[netsettler]

Suggested change

	def from_identifier_and_request(
	cls, identifier: str, request: Request, fetch_links: bool, **kwargs: Any
	) -> PortalItem:
	def from_identifier_and_request(cls, identifier: str, request: Request, fetch_links: bool,
	**kwargs: Any) -> PortalItem:

[netsettler]

I'd use a blank line after the class statement if you're going to do a definition that's at all complex. You can pack them in if the class definition is a simple set of variable setups but otherwise it gives a cluttered visual look.

Suggested change

	request: Optional[Request] = field(default=None, hash=False)
	request: Optional[Request] = field(default=None, hash=False)

[netsettler]

Suggested change

	mock_from_identifier_and_auth.assert_called_once_with(
	identifier, auth, fetch_links=fetch_links
	)
	mock_from_identifier_and_auth.assert_called_once_with(identifier, auth, fetch_links=fetch_links)

[netsettler]

Suggested change

	mock_from_identifier_and_request.assert_called_once_with(
	identifier, request_param, fetch_links=fetch_links
	)
	mock_from_identifier_and_request.assert_called_once_with(identifier, request_param,
	fetch_links=fetch_links)

[netsettler]

My only concerns about this file is the syntax. It fights conventional notation (which you're allowed to do if forced by long names or other factors, but such are not in play here), and that conventional notation allows a lot more things to appear on screen than the notation you've chosen. Conventional notation is also designed to make function calls look like function calls, with things in a certain visual orientation, and one has to step back from each of these calls in order to see what's even going on, which makes the reading and understanding of the code more complex.

There is also a more subtle issue which is that we often use grep to find uses of functions with particular arguments, and while there's no way to assure everything will occur on one line, the ability to grep for a pattern like "foo[(].*x=3" is thwarted if you guarantee that every argument to foo is always on another line. It just adds one more bit of difficulty where other tools than the easy ones must be sought to do ordinary maintenance operations.

Ordinarily, I like to give considerable latitude to the code writer/maintainer to maintain their own code in the way they want, but it's useful if there is at least some coherence to the overall code base, and I've tried to give hints here of the way most of our code base is. It allows people to move more gracefully between modules.

It's more normal as a coding style to just do

something(a, b, c)

not

something(
   a, b, c
)

by default unless it doesn't fit line length (ours is 120 characters). After that, try:

something(a, b, c,
          d, e, f)

though exactly where you break lines can vary according to local aesthetics with more variability
than I am showing.

You really only fall back to this notation you're using, with the arguments backdented from the paren like this:

something(
   a, b, c,
   d, e, f
)

if you can't get things to fit or have some strong reason to believe there will be a flow of new parameters
that will lead to that in future patches.

Note also, that if the argument order is not strongly dictated by the verb, I recommend doing:

def something(*, a, b, c):

in order to force callers to be like

something(a=..., b=..., c=...)

for example so that if you have to add args later you can put them in whatever order you want. If you have done
just

def f(a, b, c):

then in Python the caller might have done either f(1, 2, 3) or f(a=1, b=1, c=1), but the existing code calling positionally will be broken by later changes to the code that adds new arguments in the middle, but putting in the "*" to require keyword argument calling will protect you from that, since keywords can occur in any order.

[willronchetti]

Generally Doug has great style as he has an automatic linter that seems to approximately follow Google's style guide but I am in agreement with Kent on this.

[netsettler]

I'm not sure what the part where you're doing result.embed is doing here. There need to be some doc strings explaining what these methods are doing.

But the reason I noticed this is another worry I had (that the doc string might calm me about): Won't this end up calling Exception with the arguments that were given to the function? That seems a little odd to me. If there are keywords, Exception will probably fuss.
e.g.,

>>> str(Exception('foo', True))
"('foo', True)"
>>> str(Exception('foo', True, x=17))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Exception() takes no keyword arguments

I don't know in what context the MagicMock that has the .embed set will actually call that .embed.

Maybe what you're needing is:

def embedded_method(*args, **kwargs):
    return Exception(f"Attempted to all embed with args={args} kwargs={kwargs}")
embed_method.side_effect = embedded_method

though here note you are returning, not raising, the Exception. Is that what you mean to be doing? Setting it as the side-effect won't raise the exception. That's allowed, but it's also unusual. It's not a side-effect, but in mocks the .side_effect's return value is used, so it's not only a side-effect. Still, just the use of Exception usually causes people to think something will get raised unless there's a clearly documented protocol explaining why the raising is delayed or why an Exception is being used just as a kind of passive error return value.

[netsettler]

Relating to my earlier remarks about the .embed, if this is Exception an exception object is likely to just be a unique object and I suppose this is likely to just always fail, so maybe that's what you want, but that's a very odd way to do that. I don't know what I think of that. I feel like it will have some of the useful info but that the form of the error you get will be mysterious because the error message in the Exception will say some arguments but not the context in which those arguments were acquired.

[willronchetti]

Looks good but Kent's syntactic suggestions should be considered

[willronchetti]

Point of classmethod vs. staticmethod? Also maybe _get_item_view_object, or if other views desired make object a default param?

[netsettler]

FWIW, I always prefer class methods over static methods. They are equally easy to test (because you don't have to instantiate the class). The class method is more flexible in its ability to access class variables without rewriting. What meager efficiency is lost in being a class method, if any, is hardly worth discussing.