Add defaults to all the Item.from_dict pops

[jsignell]

Related Issue(s): #831

Description:
The easy solution to better error handling is to just defer until the init. So I added pop defaults to all the dict fields even if they are required

PR Checklist:

• Code is formatted (run pre-commit run --all-files)
• Tests pass (run scripts/test)
• Documentation has been updated to reflect changes, if applicable
• This PR maintains or improves overall codebase code coverage.
• Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

[jsignell]

Just noticed the comment on the other PR about pytest. I'll update this now

[codecov-commenter]

Codecov Report

Base: 90.13% // Head: 90.13% // No change to project coverage 👍

Coverage data is based on head (dfde1be) compared to base (471ceeb).
Patch coverage: 100.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #994   +/-   ##
=======================================
  Coverage   90.13%   90.13%           
=======================================
  Files          47       47           
  Lines        6164     6164           
  Branches      920      923    +3     
=======================================
  Hits         5556     5556           
  Misses        426      426           
  Partials      182      182           

Impacted Files	Coverage Δ
pystac/item.py	92.82% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[gadomski]

I'm going to link this with #831, even though it doesn't implement the fix exactly as recommended by @philvarner. IMO we don't want to doing psuedo-validation in multiple places, so if there is going to be validity checks, they should probably be in the constructor (not in from_dict). If folks want to do more checking in the constructor (e.g. ensuring that bbox is defined if geometry is), that should be done in a separate PR.

[philvarner]

Pete, I think you're getting to root of one of the issues with methods like this, that take an arbitrary input and produce a constrained/validated object. If I were going to (possibly-over) design this:

• Item would have strict value constraints such that it would not be possible to construct a syntactically-invalid Item. E.g., you couldn't construct one without a type, datetime, etc, but you could construct one that has invalid extension field values or whatever.
• A class MaybeItem with from_dict that would do it's best to construct a STAC Item (not an Item object) from the dict and always returns a MaybeItem no matter what the input dict looks like. This would be like an Item, but without all the value restrictions, e.g., I could construct one without a stac_version. Then, have a method MaybeItem.to_item that would construct an Item, but it either raises a specific Exception or a python-equivalent of Result<Item, E>, rather than incidentally raising a KeyError
• Item can still have a method to_item, but it either raises a specific Exception or a python-equivalent of Result<Item, E>, rather than incidentally raising a KeyError if there's anything invalid about the item.

[gadomski]

@philvarner I don't think the current code is that far from what you're describing. The Item constructor is doing some validation (e.g. the datetime stuff), so all we'd need to add is More Validation™ to get to your goal of a nice, strict, constrained class.

I'm not sure of the value of MaybeItem -- I think Dict[str, Any] is basically a MaybeItem. I don't quite see the utility in this (which is what I think you mean by MabyeItem):

class MaybeItem:
    type: Optional[Any]
    stac_version: Optional[Any]
    ...

Am I misunderstanding MaybeItem?

[philvarner]

I was thinking of it a bit more strict than that -- the fields value are all Optional, but of the correct type.

class MaybeItem:
type: Optional[str]
stac_version: Optional[str]
...

The use case here is the same one that Item is currently failing on -- I want to read an invalid item and then presumably do something with it while in an invalid state or modify it to be valid. When I'm modifying it, it's a nicer experience to modify the object and get the typechecking on it rather than operating on an dict[str, Any] with no checking.

[gadomski]

When I'm modifying it, it's a nicer experience to modify the object and get the typechecking on it rather than operating on an dict[str, Any] with no checking.

Fair, and makes sense. There is a tradeoff w/ complexity b/c we'd be adding another, slightly ambiguous item to the namespace. I do see the value in providing some sort of support for invalid STAC -- without it, we end up with problems like #991, where it's totally opaque to a user what the problem is w/ a STAC read operation.

My instinct is to try to ring-fence the Maybe* classes in the IO space (e.g. keep them in pystac.io), since you'd presumably be using them exclusively with stuff you find "in the wild" (i.e. things you read in). When you're making STAC yourself, you shouldn't need those potentially-invalid structures, right?

[philvarner]

several comments.