search

casework / CASE-Mapping-Python

Apache License 2.0
0 stars 3 forks source link

Fix #7 issue, changing FacetOperatingSystem #47

Closed fabrizio-turchi closed 4 months ago

fabrizio-turchi commented 4 months ago

I add all properties included in the OperatingSystemFacet class and adding the type checking.

  + os_advertisingID: str = None,
  + os_bitness: str = None,
  + os_isLimitAdTrackingEnabled: bool = None,
  + os_environment_variables: Union[None, Dict] = None

I also updated the example.py.

ajnelson-nist commented 4 months ago

This report came in from the type-checking:

case_mapping/uco/observable.py:1123: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True

(and again for each parameter)

I'm going to push two patches.

fabrizio-turchi commented 4 months ago

Thanks Alex,

I have a hazy idea of the error, I will check your changes after the pushes!

ajnelson-nist commented 4 months ago

The relevant text from the PEP 484 appears to be from this section:

One common case of union types are optional types. By default, None is an invalid value for any type, unless a default value of None has been provided in the function definition. [...] [...] A past version of this PEP allowed type checkers to assume an optional type when the default value is None [...]. This is no longer the recommended behavior. Type checkers should move towards requiring the optional type to be made explicit.

ajnelson-nist commented 4 months ago

One more patch coming.

ajnelson-nist commented 4 months ago

I've added two more patches to check for, and prevent, confusion of positional arguments vs. keyword parameters. I'm fine if you'd rather they be reverted. Otherwise, this PR looks good to me. The only ontologically-necessary change it needed was the manufacturer parameter is typed in the ontology as requiring a uco-identity:Identity, not uco-observable:ObservableObject.

ajnelson-nist commented 4 months ago

Oh wait, one more fix coming.

fabrizio-turchi commented 4 months ago

the PR It's fine, I'm only curious of the reasons why you added the parameters *args and **kwargs, did it mean for future extensions of the ontology?

ajnelson-nist commented 4 months ago

the PR It's fine, I'm only curious of the reasons why you added the parameters *args and **kwargs, did it mean for future extensions of the ontology?

No, it's a Python syntax thing, possibly just my personal preference or confusion.

A Python function can have (1) positional arguments that require a value, (2) positional arguments that specify a default, (3) keyword parameters that require a value, and (4) keyword parameters that specify a default value. If all 4 of those are specified for one function, they must be specified in that order. If (3) is skipped, the syntax appears, to me, ambiguous on when positional-with-default arguments end and keyword-with-default parameters begin. There's a behavior Python lets function-callers trigger that further makes this easier for callers when they use parameter names, but at a trade of some syntactic laxness.

*args is a marker for "There can be an arbitrary number of unnamed positional arguments here in the list; everything afterwards is a non-positional parameter." Unfortunately, I have trouble recalling which PEP shored up this syntax, and further I think it used the harder-to-text-search sole * and ** spelling (vs. *args and **kwargs); I think it was incorporated in Python 3.8?

So, apologies for lacking the citation, but the short story is, using *args, a code-reader can't confuse when it's OK to start mixing up the order of the named arguments. **kwargs does set up some future pass-through support, and I've found it handy for subclass design.

fabrizio-turchi commented 4 months ago

thanks a million for your explanation😊