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

Add an API for submitting sketches to a class #72 #145

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

myqng
Copy link

@myqng myqng commented Nov 2, 2022

No description provided.

@myqng myqng linked an issue Nov 2, 2022 that may be closed by this pull request
@timothycpoon timothycpoon self-requested a review November 2, 2022 03:34
Copy link
Contributor

@timothycpoon timothycpoon left a comment

Choose a reason for hiding this comment

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

Hey Marianne, overall this looks pretty good. There are just a couple of small typos. In the future, if you use https://golangci-lint.run/usage/install/#local-installation or something similar, these types of errors can be caught earlier. Thank you for your work! :)

Edit: One more thing, it would be good to add a small description to your PR in the future. It really helps contextualize the PR.

handler/class.go Outdated
if err := httpext.RequestBodyTo(c.Request(), &req); err != nil {
return c.String(http.StatusInternalServerError, err.Error())
}
if submissionPID.uid == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be req.submissionPID

handler/class.go Outdated
if submissionPID.uid == "" {
return c.String(http.StatusBadRequest, "submission PID is required")
}
if assignmentPID.uid == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, req.assignmentPID

if req.cid == "" {
return c.String(http.StatusBadRequest, "cid is required")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For now, if you just add return nil at the end of the function, it should work as expected

Copy link
Contributor

@timothycpoon timothycpoon left a comment

Choose a reason for hiding this comment

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

Hey Marianne, there are a couple more compile errors. I understand that it's not very simple to test a new API like this, but here are some basic steps you can take to "soft verify" any changes:

  • building and running the server should happen without any errors
  • go fmt should run without any errors
  • tests on GitHub should not fail

This may not catch all the errors, but some obvious ones like typos, etc can be found this way.

if err := httpext.RequestBodyTo(c.Request(), &req); err != nil {
return c.String(http.StatusInternalServerError, err.Error())
}
if req.submissionPID.uid == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if req.submissionPID.uid == "" {
if req.submissionPID == "" {

if req.submissionPID.uid == "" {
return c.String(http.StatusBadRequest, "submission PID is required")
}
if req.assignmentPID.uid == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if req.assignmentPID.uid == "" {
if req.assignmentPID == "" {

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.

Add an API for submitting sketches to a class
2 participants