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

[Refactor][Misc] Cleaned up critical capture code. #663

Merged
merged 5 commits into from
Feb 17, 2025

Conversation

frutescens
Copy link
Contributor

What are the changes the user will see?

None.

Why am I making these changes?

With this codebase, there's moments where you open a random file and see a stupid, chunky ternary.

What are the changes from a developer perspective?

Ternary broken up into if-else statements for better readability.

How to test the changes?

Critical Captures should work as expected.

Checklist

  • I'm using beta as my base branch
  • There is no overlap with another PR?
  • The PR is self-contained and cannot be split into smaller PRs?
  • Have I provided a clear explanation of the changes?
  • Have I tested the changes manually?
  • Are all unit tests still passing? (npm run test:silent)
    • Have I created new automated tests (npm run create-test) or updated existing tests related to the PR's changes?

@frutescens frutescens added Refactor For refactoring code Miscellaneous Changes that don't fit under any other label labels Feb 13, 2025
Tempo-anon
Tempo-anon previously approved these changes Feb 13, 2025
Copy link
Contributor

@Tempo-anon Tempo-anon left a comment

Choose a reason for hiding this comment

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

chained ternaries are a crime

DayKev
DayKev previously approved these changes Feb 14, 2025
innerthunder
innerthunder previously approved these changes Feb 14, 2025
Co-authored-by: NightKev <34855794+DayKev@users.noreply.github.com>
@frutescens frutescens dismissed stale reviews from innerthunder, DayKev, and Tempo-anon via 8f9599e February 16, 2025 05:19
DayKev
DayKev previously approved these changes Feb 16, 2025
Copy link
Contributor

@MokaStitcher MokaStitcher left a comment

Choose a reason for hiding this comment

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

I'm assuming the change of one threshold from 100 to 40 was intended?
If yes that also becomes a balance PR and i'd like to know why/where this was decided and have that information added in the PR description please

@MokaStitcher MokaStitcher merged commit c2f0410 into beta Feb 17, 2025
8 checks passed
@MokaStitcher MokaStitcher deleted the critCapturesFix branch February 17, 2025 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Miscellaneous Changes that don't fit under any other label Refactor For refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants