WIP: Add support for Secret Incidents API endpoints #84
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #84 +/- ##
==========================================
+ Coverage 92.28% 93.29% +1.01%
==========================================
Files 6 11 +5
Lines 622 895 +273
==========================================
+ Hits 574 835 +261
- Misses 48 60 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
irgeek
force-pushed
the
add-incidents
branch
from
22f32c8 to
d3d778e
Compare
|
The |
There was a problem hiding this comment.
Wow, that's some huge work, thanks!
I made some remarks inline, but here are some more general ones:
I'd rather avoid too deep file hierarchies and make the following changes:
- merge all incident_models code into an incident_models.py module, similar to what you did with source_models.py.
- merge utils/tools.py into utils/init.py
I think an utils package should not contain model-specific code so I would move load_incident_response() to incident_models.py.
I am not fond of the _url param added to GGClient.request(). I understand this is required to support paging, but I'd rather not add such a parameter to py-gitguardian public API. Maybe you can take the bulk of the requests() method minus the endpoint-to-url code, move it to utils and have both GGClient.request() and code using paging to use it?
Some of the functions you added take a lot of optional arguments. You should make them keyword-only arguments.
| @@ -5,6 +5,7 @@ verify_ssl = true | |||
|
|
|||
| [packages] | |||
| pygitguardian = { editable = true, path = "." } | |||
| strenum = "*" | |||
There was a problem hiding this comment.
Adding StrEnum here is not required: non-dev dependencies only need to be in setup.py.
| "typing-extensions", | ||
| "urllib3<2", | ||
| "strenum", |
There was a problem hiding this comment.
Can you pin StrEnum to >=0.4, <0.5 (even if you are very close to StrEnum maintainer 😉)?
| present = auto() | ||
| removed = auto() |
There was a problem hiding this comment.
Can you make these values upper-case, like the others?
| def __int__(self): | ||
| return self.id |
There was a problem hiding this comment.
I'd rather not have such magic behavior, using .id is simple enough.
|
|
||
|
|
||
| def count_not_none(seq: Sequence[Any]) -> int: | ||
| """Count the number of None values in a sequence""" |
There was a problem hiding this comment.
| """Count the number of None values in a sequence""" | |
| """Count the number of not-None values in a sequence""" |
|
|
||
| def ensure_mutually_exclusive(msg: str, *seq: Any) -> None: | ||
| """ | ||
| Ensure only one value in a sequence is None. |
There was a problem hiding this comment.
| Ensure only one value in a sequence is None. | |
| Ensure only one value in a sequence is not None. |
|
|
||
| def count_not_none(seq: Sequence[Any]) -> int: | ||
| """Count the number of None values in a sequence""" | ||
| return sum(0 if v is None else 1 for v in seq) |
There was a problem hiding this comment.
nit: can be slightly simplified:
| return sum(0 if v is None else 1 for v in seq) | |
| return sum(1 for v in seq if v is not None) |
| type: str # TODO: Reserved word | ||
| full_name: str | ||
| health: SourceHealth = field(metadata={"by_value": True}) | ||
| open_incidents_count: int # TODO: Type documented as "number" - what's the difference? | ||
| closed_incidents_count: int # TODO: Also "number" | ||
| visibility: str # TODO: Really? str |
There was a problem hiding this comment.
Are those TODO actionable? If not, better remove them.
|
|
||
|
|
||
| @dataclass | ||
| class Match(Base, FromDictMixin): |
There was a problem hiding this comment.
There is already a Match class in models, can we name this one OccurrenceMatch?
|
Sorry I have to close this PR. It has not moved for several months now. Meanwhile incident support has just been added by #114. |
The PR adds models and methods to handle the Secret Incidents API endpoints. It still needs a changelog entry and (probably) some cleanup, but I wanted to get some feedback before I put too much work into it. As requested I've sliced the changes up in to several commits for ease of review.