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

Tidy up LegacyLastTickOffset usages and stop passing everywhere #24965

Merged
merged 3 commits into from
Oct 4, 2023

Conversation

peppy
Copy link
Member

@peppy peppy commented Sep 29, 2023

Initial code quality / cleanup pass for #22805, before making any logic changes to this.

There should be no behavioural changes as a result of this change.

@bdach
Copy link
Collaborator

bdach commented Sep 29, 2023

For all intents and purposes I don't see this changing anything but may run a sheet for this one anyways just to be on the safe side.

@peppy
Copy link
Member Author

peppy commented Sep 29, 2023

For all intents and purposes I don't see this changing anything but may run a sheet for this one anyways just to be on the safe side.

If we must, then I'd run it against #24966 instead and hold off merging this until confirmed. But I don't see how this could affect difficulty calculation.

@bdach
Copy link
Collaborator

bdach commented Sep 29, 2023

Sure, I can do that instead. Generally want to rack @smoogipoo's brain on this one anyways after the last time we did a "legacy" prefix removal on a thing.

@smoogipoo
Copy link
Contributor

smoogipoo commented Oct 2, 2023

I'm running a diffcalc test on this right now at https://github.com/smoogipoo/ga-diffcalc-tester/actions/runs/6380004379, though I don't know if it'll complete successfully at the moment 😅

@peppy
Copy link
Member Author

peppy commented Oct 2, 2023

I'm running a diffcalc test on this right now at smoogipoo/ga-diffcalc-tester/actions/runs/6376069767, though I don't know if it'll complete successfully at the moment 😅

Did you see #24965 (comment)? Not sure on the value of running on this PR, but I guess it's fine.

@smoogipoo
Copy link
Contributor

smoogipoo commented Oct 2, 2023

Yeah I’m running it on the follow up PR, but posted here to be relevant for the above comment.

Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

This is fine to get in on its own.

@peppy peppy merged commit 72cbc3b into ppy:master Oct 4, 2023
14 of 17 checks passed
@peppy peppy deleted the legacy-tick-not-so-legacy-after-all branch October 4, 2023 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants