Add support for Timeline events #366
Conversation
|
I'm currently running this code on a lot of issues in flutter/flutter to see if there are any crashes or whatnot. I'm very happy to adjust this code in whatever way is appropriate; I wasn't sure about the conventions and so forth. For example, I wasn't sure how to test this, but couldn't find any tests for similar code. Is it even testable given that all that matters is if it works against the GitHub servers and we can't very well be running the tests against that? Also I wasn't sure what to do about documentation and so on. I don't really know what many of these fields actually are, the GitHub documentation is... not detailed and is contradictory in places. Fixes #364 |
Hixie
force-pushed
the
timeline
branch
2 times, most recently
from
b96e707 to
46f7314
Compare
This is primarily based on the timeline API response schema (using code generated from the schema by the script in `tool/process_github_schema.dart`). Where I could find existing model classes compatible with bits of that schema I tried to re-use them, extending them as necessary. I took some liberties where the GitHub API documentation was vague, contradictory, or otherwise dubious. The comments on the new code are based on the comments in the response schema. They're not super helpful, if I'm honest. I found that some of the model classes were inconsistent in various ways and tried to clean up the code near what I touched to be more consistent. For example, I made a bunch of fields on a bunch of model classes mutable where previously they were `final`, to be more consistent with other model classes. I also moved some constructors to be above their corresponding fields to be more consistent with other code that already did that.
|
Thanks for putting up the PR. I will review it and give it some testing on my end as well. The automated testing story in this project isn't where I'd like it to be yet so I often rely on people testing manually. |
|
FWIW I used this for every issue and PR in the main repos of |
There was a problem hiding this comment.
🌮 That was a lot to review but this looks good to me. I appreciate reusing existing classes and adding properties where needed. One thing I admittedly only lightly reviewed was the process_github_schema.dart script. I'll have to play around with that more. It could end up being quite useful in the future.
robrbecker
added
semver:minor
|
Released in version 9.12.0 https://github.com/SpinlockLabs/github.dart/releases/tag/9.12.0 |
This is primarily based on the timeline API response schema (using code generated from the schema by the script in
tool/process_github_schema.dart). Where I could find existing model classes compatible with bits of that schema I tried to re-use them, extending them as necessary. I took some liberties where the GitHub API documentation was vague, contradictory, or otherwise dubious.The comments on the new code are based on the comments in the response schema. They're not super helpful, if I'm honest.
I found that some of the model classes were inconsistent in various ways and tried to clean up the code near what I touched to be more consistent. For example, I made a bunch of fields on a bunch of model classes mutable where previously they were
final, to be more consistent with other model classes. I also moved some constructors to be above their corresponding fields to be more consistent with other code that already did that.