Improve solution collector #818
Conversation
unrealhoang
commented
Jul 23, 2018
unrealhoang
commented
- Refactor
- Add structopt for params parsing
- Concise submit parsing
solution_collector/src/main.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug)] | ||
| struct SubmitEntry { |
There was a problem hiding this comment.
@unrealhoang Could you add a field for submitted date, and using this field for checking a PR is submitted after the deadline?
There was a problem hiding this comment.
nice catch, will add it.
| println!("Fetching Page {}", page); | ||
|
|
||
| let (_, _, pulls) = client.get() | ||
| let (_, _, pulls) = ctx.client.get() |
There was a problem hiding this comment.
Could you filtered out all PR that are submitted after the deadline, or better mark them as due-date :D
|
Should we bump the version to |
| }, | ||
| #[structopt(name = "merge")] | ||
| Merge { | ||
| week: u32, |
There was a problem hiding this comment.
I don't see any logical difference between WeekReport::week and Merge::week. Aren't they supposed to be the same?
There was a problem hiding this comment.
@huytd: week report is for, well, week report 😂. It will only show that week’s submissions, compare them against PARTICIPANTS list, group submitted entries by number of solved problem.
solution_collector/src/main.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug)] | ||
| struct Problem { |
There was a problem hiding this comment.
We need a better name so it should not be confusing. For example, Submission (instead of SubmitEntry), and Entry (instead of Problem).
Using Problem here makes me think about a data structure to store parser's error results.
solution_collector/src/main.rs
Outdated
| }).collect() | ||
| } | ||
|
|
||
| fn parse_pull_request(pr: &Value) -> Option<SubmitEntry> { |
There was a problem hiding this comment.
Or we can just name it PullRequest instead of SubmitEntry
| } | ||
|
|
||
| fn get_week(title: &str) -> u64 { | ||
| fn get_week(title: &str) -> u32 { |
There was a problem hiding this comment.
Should we name this funciton parse_week, and instead of returning 0 if we cannot parse, should we return Option<u32>?
There was a problem hiding this comment.
upvote for parse_week, so it would be consistent with other function names in this impl.
