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

Parts and robots #46

Merged
merged 29 commits into from
Jun 5, 2024
Merged

Parts and robots #46

merged 29 commits into from
Jun 5, 2024

Conversation

chennisden
Copy link
Contributor

glue together frontend and non user api stuff

@chennisden
Copy link
Contributor Author

fixing merge

Copy link
Member

@codekansas codekansas left a comment

Choose a reason for hiding this comment

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

nice

Comment on lines 69 to 78
const response = await this.api.get(`/robots/${robotId}`);
return response.data;
} catch (error) {
if (axios.isAxiosError(error)) {
console.error("Error fetching robot:", error.response?.data);
throw new Error(error.response?.data?.detail || "Error fetching robot");
} else {
console.error("Unexpected error:", error);
throw new Error("Unexpected error");
}
Copy link
Member

Choose a reason for hiding this comment

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

this pattern is repeated 3 times, consider refactoring into a helper function (might need to do some typing voodoo to make work)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i feel like the effort to make it "generic" (there are a lot of subtly different args) would not be worthwhile atm

frontend/src/pages/PartDetails.tsx Outdated Show resolved Hide resolved
purchase_links: part.purchase_links,
used_by: part.used_by,
};
/*
Copy link
Member

Choose a reason for hiding this comment

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

can just delete instead of commenting out

@@ -0,0 +1,111 @@
import axios from "axios";
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to auto-generate these interfaces from the openapi.json file for the backend?

frontend/src/pages/RobotDetails.tsx Outdated Show resolved Hide resolved
store/app/api/routers/main.py Outdated Show resolved Hide resolved
store/app/api/routers/main.py Outdated Show resolved Hide resolved
store/app/api/routers/main.py Outdated Show resolved Hide resolved
@@ -12,7 +12,7 @@

from store.app.api.crypto import get_new_api_key, get_new_user_id
from store.app.api.db import Crud
from store.app.api.email import OneTimePassPayload, send_delete_email, send_otp_email
from store.app.api.email_utils import OneTimePassPayload, send_delete_email, send_otp_email
Copy link
Member

Choose a reason for hiding this comment

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

we can instead make a utils dir and move this to utils/email.py

store/settings/configs/local.yaml Outdated Show resolved Hide resolved
@chennisden
Copy link
Contributor Author

Yeah there's a lot of accumulated cruft everywhere because this branch diverged from master a while ago. A lot of these things are actually regressions that I think we just failed to spot

Copy link
Member

@codekansas codekansas left a comment

Choose a reason for hiding this comment

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

request

frontend/src/App.tsx Outdated Show resolved Hide resolved
@chennisden
Copy link
Contributor Author

@codekansas ready to merge

@chennisden
Copy link
Contributor Author

(refactoring stuff inot crud will be next pr)

@codekansas codekansas merged commit 9f6032e into master Jun 5, 2024
1 check passed
@codekansas codekansas deleted the parts_and_robots branch June 5, 2024 21:48
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.

3 participants