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.

Sign up for GitHub

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

Jump to bottom

refactor: Rename "finish" variables and functions to "review" #2673

Merged
merged 1 commit into from
Jun 25, 2024
Merged

refactor: Rename "finish" variables and functions to "review" #2673

merged 1 commit into from
Jun 25, 2024

Conversation

joshlarson
Copy link
Contributor

@joshlarson joshlarson commented Jun 24, 2024 •
edited
Loading

@joshlarson joshlarson marked this pull request as ready for review June 24, 2024 20:58
@joshlarson joshlarson requested a review from a team as a code owner June 24, 2024 20:58
@joshlarson joshlarson marked this pull request as draft June 24, 2024 21:01
@joshlarson joshlarson marked this pull request as ready for review June 24, 2024 21:02
@github-actions GitHub Actions
Copy link

Coverage of commit 72be114

Summary coverage rate:
  lines......: 93.6% (3271 of 3493 lines)
  functions..: 73.5% (1346 of 1831 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

firestack
firestack approved these changes Jun 24, 2024
View reviewed changes
Copy link
Member

@firestack firestack 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 a little iffy on this, as I'd love to find a name that doesn't need to change with the copy, but while our functions match the copy, this is at least an improvement.

@joshlarson
Copy link
Contributor Author

I'd love to find a name that doesn't need to change with the copy

Yeah I'm kind of torn, but since there's such a close mapping between states and copy right now (because in a sense, the whole point of the states is to determine what shows up on the screen), I wasn't sure what else to do.

I also really didn't like the term Finish there, since it has always kind of felt misleading.

@joshlarson joshlarson merged commit 6d77ac4 into main Jun 25, 2024
54 checks passed
@joshlarson joshlarson deleted the jdl/refactor/rename-finish-internals-to-review branch June 25, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@firestack firestack firestack approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joshlarson @firestack