Skip to content

fix: types for logging integration args#444

Merged
untitaker merged 3 commits into
getsentry:masterfrom
sbdchd:steve/fix-logging-integration-types
Aug 5, 2019
Merged

fix: types for logging integration args#444
untitaker merged 3 commits into
getsentry:masterfrom
sbdchd:steve/fix-logging-integration-types

Conversation

@sbdchd
Copy link
Copy Markdown
Contributor

@sbdchd sbdchd commented Jul 28, 2019

The sdk supports disabling the LoggingIntegration by passing None as
arguments; however, the current types require ints.

This behavior is noted in the docs:
https://docs.sentry.io/platforms/python/logging/#options

sentry_sdk.init(integrations=[LoggingIntegration(level=None, event_level=None)])

edit: when adding # type: ignore I found a second issue with the argument integrations which mypy recommended as typing as Sequence instead of List. This fixed the type error.

sbdchd added 3 commits July 27, 2019 21:17
The sdk supports disabling the LoggingIntegration by passing `None` as
arguments; however, the current types require ints.

This behavior is noted in the docs:
https://docs.sentry.io/platforms/python/logging/#options

```
sentry_sdk.init(integrations=[LoggingIntegration(level=None, event_level=None)])
```
```python
integrations = [
    LoggingIntegration(level=None, event_level=None)  # type: ignore
]
sentry_sdk.init(integrations=integrations)
```

here are the type errors from mypy

```
Argument "integrations" to "init" has incompatible type "List[LoggingIntegration]"; expected "List[Integration]"
"List" is invariant -- see http://mypy.readthedocs.io/en/latest/common_issues.html#variance
Consider using "Sequence" instead, which is covariant
```
@untitaker
Copy link
Copy Markdown
Member

What do you pass to integrations other than a list? I'd like to prevent people from passing in non-resettable iterators because I make no guarantees about how often I iterate over the sequence.

@untitaker
Copy link
Copy Markdown
Member

Nvm sequence seems fine for this.

@bluetech
Copy link
Copy Markdown
Contributor

The changes LGTM.


With regards to List vs Sequence, the rules I try to follow is:

For inputs, e.g. function arguments, prefer Sequence, because it accepts more types, and provides more guarantees to the caller, namely that the sequence is not mutated (append, pop, del and friends).

For outputs, give the specific type (e.g. List), so the caller can do whatever they want with it. One exception is that if the API wants to retain the option to return some other sequence type, then it should use a more general type.

Also, when taking a callback, the situation is reversed.

@untitaker untitaker merged commit 0718288 into getsentry:master Aug 5, 2019
@untitaker
Copy link
Copy Markdown
Member

Thanks!

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.

3 participants