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

add condition to get commits within 30 days of the latest commit #1921

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

s7tya
Copy link
Contributor

@s7tya s7tya commented Jun 9, 2024

closes #1920

I'm not sure if this is a good implementation.

@Kobzol
Copy link
Contributor

Kobzol commented Jun 10, 2024

This is a bit hacky, yeah 😅 And it won't work for locally benchmarked commits, since these are not master commits.

Ideally, we should move the hardcoded condition that None as a left bound means "now() - 30 days" from within the Bound implementation itself, and just treat bound=none, left to be the first commit, and bound=none, right to be the last commit. But I'm a bit wary of doing that, because the bounds are used in a bunch of places, and doing this change might break a lot of stuff.

I realize now that it's quite difficult to make this work both locally and on the live server, because there we have very different requirements. On perf.RLO, we want to show only master commits, with a default range of [-30 days, now()]. Locally, we want to show all commits, with a default range of [first commit, last commit] (or [-30 days, last commit]). While the [-30 days, last commit] bound could work both for local and perf.RLO usage, the is_master() condition is a problem.

I'm not really sure what to do with that. We could display also try/local commits e.g. in debug mode, but that's quite unintuitive. We could add some parameter to the website, or check if the URL e.g. starts with localhost, but even then I wouldn't want to check this condition in Bound::[left/right]_match, but rather in comparison.rs. But for that we'd probably need to refactor the bounds quite a lot, add information to them if they should keep only master commits etc.

Maybe we could modify collector so that it puts local commits into the DB as master commits, and fills out the date for them 🤔 That could help unifying the local and production environments.

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.

Show existing data when opening the site locally
2 participants