Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add event stats #16

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

WillLillis
Copy link
Contributor

@WillLillis WillLillis commented Nov 19, 2023

As suggested in CONTRIBUTING.md, this feature adds the ability to display the Event Stats for a given year.

I've tested this on all years and it appears to be functioning correctly, but I'm happy to refactor/ fix things if they're not up to standard :)

Screenshot of 2022:
image

.get(url)
.send()?;
let contents = response.error_for_status()?.text()?;

Copy link
Owner

@scarvalhojr scarvalhojr Nov 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to handle 404 here as well, which can happen if you request stats from future events (say, 2024).

let star_val = stats_text
.split_whitespace()
.find(|chunk| chunk.parse::<u64>().is_ok())
.unwrap()
Copy link
Owner

@scarvalhojr scarvalhojr Nov 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unwrap is a bit dangerous. We need to handle it in case the page format changes in the future (we could return a AocResponseError to flag this). For instance, this currently panics if you ask for the 2023 stats, which gets an almost blank page with a "no stats yet" message. We could use a regex to make sure we get the right number looking for something like represents up to <number> users

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I initially avoided using a regex because I don't have any experience using them (yet), but I'll look into this. Thanks!

Comment on lines 478 to 480
let split: Vec<&str> = line.split_whitespace().collect();
let gold_comps = split[1].parse::<usize>().unwrap();
let n_stars = split[3].len();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also assumes the page format won't change. Maybe a regex would be safer (and return error if it doesn't match).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise here, I will start working on switching this functionality over to a regex. Thanks so much for the feedback!

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.

2 participants