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

Drop sqlc overrides for JobState #229

Merged
merged 1 commit into from
Feb 25, 2024
Merged

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Feb 25, 2024

This is another small one, but I noticed while doing driver work that
since we put a layer of abstraction between sqlc and any public-facing
APIs, the JobState* state constants in sqlc are not really used
anymore. Even the JobState type is used to infrequently that golfing
"River" off the front also does very little.

Here, I propose that we just get rid of the sqlc overrides. They're not
a big deal, but not having them means less sqlc configuration that needs
to be shared around between YAML files, and less opportunity for error
there.

This is another small one, but I noticed while doing driver work that
since we put a layer of abstraction between sqlc and any public-facing
APIs, the `JobState*` state constants in sqlc are not really used
anymore. Even the `JobState` type is used to infrequently that golfing
"River" off the front also does very little.

Here, I propose that we just get rid of the sqlc overrides. They're not
a big deal, but not having them means less sqlc configuration that needs
to be shared around between YAML files, and less opportunity for error
there.
@brandur brandur requested a review from bgentry February 25, 2024 06:29
@brandur
Copy link
Contributor Author

brandur commented Feb 25, 2024

@bgentry Another small one. Seem okay?

river_job_state_discarded: "JobStateDiscarded"
river_job_state_retryable: "JobStateRetryable"
river_job_state_running: "JobStateRunning"
river_job_state_scheduled: "JobStateScheduled"
Copy link
Contributor

Choose a reason for hiding this comment

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

one less thing to have to fix when I get that pending state added soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, totally. Should keep future changes of that sort a bit easier.

@brandur brandur merged commit 0c8cadc into master Feb 25, 2024
10 checks passed
@brandur brandur deleted the brandur-drop-sqlc-constants branch February 25, 2024 23:24
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