Fix Issue Timeline exceptions by adding new EventInfoState values #1563
Conversation
…wer event types that are failing to deserialize
…d 20 open Issues in octokit.net in attempt to more likely encounter a new EventType enum value added by github api changes
|
If someone can have a 👀 over this before I merge it, that would be great! 😀 |
Octokit/Models/Response/EventInfo.cs
Outdated
| /// A line comment was made. | ||
| /// </summary> | ||
| [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Linecommented")] | ||
| Linecommented, |
There was a problem hiding this comment.
Could these be cased as LineCommented and CommitCommented? Or is this a deserialization issue?
There was a problem hiding this comment.
The API value is all lowercase, pretty sure if we do camel case on our side we'll be looking for an underscore in the API value
There was a problem hiding this comment.
@ryangribble I thought we could do something like this here?
[Parameter(Value = "linecommented")]
LineCommented,
[Parameter(Value = "commitcommented")]
CommitCommentedThere was a problem hiding this comment.
Yeah so on looking into this more I realised we dont even need to tag with the Parameter() since these are only response enums and our deserializer removes underscores and does a case insensitive Enum.Parse() anyway 👍
| { | ||
| var timelineEventInfos = await _issueTimelineClient.GetAllForIssue("microsoft", "vscode", issue.Number); | ||
|
|
||
| Assert.NotNull(timelineEventInfos); |
|
@ryangribble left a couple of minor comments, everything else is 💎 |
|
@ryangribble just a suggestion, I think it's easier to suppress CA warnings as project scope instead of class/method/property scope for CA rule of 1704. 🙂 |
I disagree. The explicitness of this rule means violations (whether necessary or otherwise) are easier to identify. Doing things at a project level makes it easier to ignore these sorts of issues. |
|
@shiftkey should be good now. I also agree on minimising the number of code analysis rules we "opt out" of at a project level 👍 |
|
@ryangribble ugh, CI failures on macOS
|
|
kicked the builds and they are ✅ now |
|
@shiftkey not sure if you wanted to close out reviewing this or not. Ill merge it in a day or two if I haven't heard back |
|
release_notes: Fix more |
nickfloyd
added
Type: Bug
Fixes #1559
Fixes #1525
This fix adds 3 more undocumented event types in the TimeLine API:
linecommentedcommitcommentedandreviewedAlso added an integration test that will hopefully help with identifying more missing values in the future, it now runs against the most recent 20 closed PRs and 20 open issues in a heavy activity repository.
I picked Microsoft/vscode 😀 the theory being that it's a very active repository, and the most recent activity is more likely to have new event types in their timeline/event stream.
The strategy seemed to work, as I discovered 2 extra event types that hadn't been reported by users as causing a crash yet 😸
In the short term, we will still throw an exception whenever we encounter an event type not previously implemented, a potential longer term solution is being hashed out in #1504