Skip to content
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.

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

Fix #7 issue, changing FacetOperatingSystem #47

Merged
merged 6 commits into from
May 2, 2024

Conversation

fabrizio-turchi
Copy link
Contributor

@fabrizio-turchi fabrizio-turchi commented May 2, 2024

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
Copy link
Member

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
Copy link
Contributor Author

Thanks Alex,

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

A follow-on patch will regenerate Make-managed files.

References:
* https://peps.python.org/pep-0484/#union-types

Signed-off-by: Alex Nelson <[email protected]>
@ajnelson-nist
Copy link
Member

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
Copy link
Member

One more patch coming.

This is to test if the current spelling in the function can tolerate
order changes.

No effects were observed on Make-managed files.

Signed-off-by: Alex Nelson <[email protected]>
No effects were observed on Make-managed files.

Signed-off-by: Alex Nelson <[email protected]>
@ajnelson-nist
Copy link
Member

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
Copy link
Member

Oh wait, one more fix coming.

No effects were observed on Make-managed files.

Signed-off-by: Alex Nelson <[email protected]>
Copy link
Member

@ajnelson-nist ajnelson-nist left a comment

Choose a reason for hiding this comment

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

@fabrizio-turchi , if you agree with the change I suggested adding *args, please feel free to merge. Because it wasn't strictly necessary (the patch just prior to its add confirms order can be tweaked), I'm fine with you asking that change be reverted.

@fabrizio-turchi
Copy link
Contributor Author

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?

@fabrizio-turchi fabrizio-turchi merged commit f5eb274 into casework:main May 2, 2024
2 checks passed
@ajnelson-nist
Copy link
Member

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
Copy link
Contributor Author

thanks a million for your explanation😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants