-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implementation of the /api/days
method, some documentation updates
#28
base: master
Are you sure you want to change the base?
Conversation
… cases Also added some details on 400/412 responses to the general methods description above
- Added a clarification of the status selection when there are multiple errors - [BACKWARD COMPATIBILITY BREAKING] Updated date format in the docs of `/api/days` - Added much more details about errors which may occur in `/api/days` - Added the `error_type` field to the response of `/api/days`, helping to identify the error occurred during a request processing based on the response - Moved response description to the "Response" subsection of the docs of `/api/days`
Wow, it's so big, there are so many TODOs... I haven't even checked that this is working yet
src/webhandlers.go
Outdated
// TODO: the two lines below are formatted automatically this way. I wonder if it is possible to write it better | ||
if len(r.Form["activity_type"]) != len(r.Form["activity_proportion"]) || | ||
len(r.Form["emotion_type"]) != len(r.Form["emotion_proportion"]) { | ||
|
||
js, err := json.Marshal(map[string]interface{}{"ok": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DarkSquirrelComes, I am planning to request your complete review on the PR when I finish it, but I have a question so far. I hate how these lines look together (especially if I remove the blank line). Is there a nice way to fix it? Our hard limit is 120 characters per line (including the indentation, my tab size is 4)
I would also be thankful if you have checked other TODOs added in this PR and tell me if there is something you disagree with or know a better way for. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This autoformat seems good-looking to me. But I don't really understand, why you compare lens of this values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This autoformat seems good-looking to me.
Then should I level the empty line there? Look what happens if I remove it:
// TODO: the two lines below are formatted automatically this way. I wonder if it is possible to write it better
if len(r.Form["activity_type"]) != len(r.Form["activity_proportion"]) ||
len(r.Form["emotion_type"]) != len(r.Form["emotion_proportion"]) {
js, err := json.Marshal(map[string]interface{}{"ok": false,
I can't understand where the condition of this if
ends and the body begins based on the indentation!
But I don't really understand, why you compare lens of this values?
Each activity/emotion here is described with one type and one proportion. I ensure there exists a type for each proportion and a proportion for each value. You can see more details on this method in the documentation
Still don't fully understand the rules of Markdown indentation... Hope, this is correct (oops, accidentally fixed an older issue...)
Adding new days is now done via one transaction Co-authored-by: Nikolay Nechaev <[email protected]>
Only activities and emotions which belong to user X can now be added to day owned by user X Co-authored-by: Nikolay Nechaev <[email protected]>
Updated the way to retrieve the id of the added day, added duplicated day error handling
…ode more specific Also added a comment about the constraints names to the scheme file
# Conflicts: # database_scheme.sql # src/webhandlers.go
…k in `HandleAPIDays` Compare the value to the `MaxProportion` constant, not to the number `100`
53cdcc3
to
aeb1c6e
Compare
…/days` This is not implemented yet
"Type id" is an object in the SQL scheme, which is called "type" in the API docs
That's pretty much it. Waiting for @VladimirKoryakin to resolve #29. I would also like the reviewers (@DarkSquirrelComes, @tanya-kta) to pay additional attention to comparing the documented and the actual behaviours of the method in this PR! Please, ensure all the errors expected to be returned are returned in all the appropriate cases |
Note: I want both two reviews on this |
One more thing: the database schema is invalid because PostgreSQL does not support |
Relates to #19
So sorry I make backwards-incompatible updates of the docs, while front-end is being built on top(?) of them! :'(
Apparently, I didn't think deep enough when drawing the documentation up. There are mostly clarifications or new details added, but there are currently at least two incompatible fixes: I replaced the
400 Bad Request
status code with412 Precondition Failed
for some cases and updated date format for one method. So, @tanya-kta and anyone else who needs the documentation, please, take a look at the changes and tell me if there is something unclear about what I've added (it's a bit late now, so I might have written some nonsense or explained something too complicatedly)Welcome the new method for adding new days!
Is blocked by #29 because of the following:
life_calendar/src/webhandlers.go
Lines 118 to 120 in b31770c