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

fix: fail fast when api key is missing and once is set #274

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

smoya
Copy link
Contributor

@smoya smoya commented Dec 3, 2024

No description provided.

@smoya smoya requested a review from a team as a code owner December 3, 2024 17:49
@smoya smoya force-pushed the voyageai-tests branch 2 times, most recently from 8751518 to 69a51bc Compare December 3, 2024 18:03
@@ -286,6 +286,9 @@ def vectorizer_worker(
for vectorizer_id in valid_vectorizer_ids:
vectorizer = get_vectorizer(db_url, vectorizer_id)
if vectorizer is None:
log.error("error fetching vectorizer", vectorizer_id=vectorizer_id) # noqa: E501 (line too long)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this makes me feel like it's about "fetching the vectorizer from the db", but as we discussed, there can be two reasons for a None value here. I would suggest making this error message a bit more generic. I miss a go-like val, err := ... pattern here though, which would propagate a more meaningful error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a nit imho; it is an important suggestion because I kinda felt the same when writing this line but then my brain decided it was good enough.

I miss a go-like val, err := ... pattern here though, which would propagate a more meaningful error.

Totally agree. I miss it in almost every language other than, eh... actually Go. Many people hate it but I love it because you have control and can acknowledge any error.

I believe the closest idiomatic behavior would be to raise an exception and catch it. I don't find it a bad solution in fact in this piece of code.

Base automatically changed from jg/voyageai-vectorizer to main December 5, 2024 08:45
@JamesGuthrie JamesGuthrie force-pushed the voyageai-tests branch 2 times, most recently from b28124b to 3b4f56d Compare December 5, 2024 09:05
@JamesGuthrie JamesGuthrie merged commit 1c2ff20 into main Dec 5, 2024
5 checks passed
@JamesGuthrie JamesGuthrie deleted the voyageai-tests branch December 5, 2024 14:04
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