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

rest api design document created #25

Merged
merged 5 commits into from
Nov 28, 2018
Merged

rest api design document created #25

merged 5 commits into from
Nov 28, 2018

Conversation

perrymant
Copy link
Owner

@perrymant perrymant commented Nov 18, 2018

  1. Layout: I had a hard time making "readable/aesthetically pleasing" markdown output, because of conflicts with how the JSON is laid out. It's almost better reading the raw markdown as the document. I wonder if you know how I could better lay this out.

  2. Content: Besides layout, I hope I have included the correct information for the API design. Can you point me to anything I'm missing. The sprint task is below for convenience.

  3. Learning Notes: I've also made some learning notes on API design (This resides in my Learning Notes repo at: https://github.com/perrymant/learning-notes/blob/master/17-rest-api-design.md.) Should I have branched this new document and PR'ed it rather than just add it to master?

Sprint task: "API design. Identify the functionalities of my current CLI and map these to HTTP REST endpoints. What should the URL look like, the HTTP methods, what is the structure of the JSON for both requests and responses. Look at server codes, and put these in the design."

Copy link
Collaborator

@kinbiko kinbiko left a comment

Choose a reason for hiding this comment

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

Good first draft. I like that you based your understanding of REST interfaces on one resource. The reason is that there are so many opinions on the matter, and trying to make sense and evaluate all of these is just going to waste your time. I've tried to make sure the comments I raised were not too contentious, opinion-wise.

  1. I tried to help you out with JSON formatting. Markdown, and JSON in markdown is not usually a problem.
  2. I think your content looks fine.
  3. Yeah, PRs in the future would be good.

design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
@perrymant
Copy link
Owner Author

I think I've addressed the improvements you have identified in this PR.

Copy link
Collaborator

@kinbiko kinbiko left a comment

Choose a reason for hiding this comment

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

Much better 👍

design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
@perrymant
Copy link
Owner Author

  1. report and transaction JSON updated
  2. Makes more sense now about thinking what a client should receive and have to deal with.

Copy link
Collaborator

@kinbiko kinbiko left a comment

Choose a reason for hiding this comment

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

Much better! Almost there now.

design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
design/api-specification-design.md Outdated Show resolved Hide resolved
@perrymant
Copy link
Owner Author

  • s/fulfill/completed
  • formatted the JSON values
  • removed excessive whitespace

Copy link
Collaborator

@kinbiko kinbiko left a comment

Choose a reason for hiding this comment

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

One more comment, then I think we're done.

},
...
]
"data" : {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't think we need this outer 'data' field.

Let's have an actual example here. You can document each field individually outside of this payload if you want to. Think of this as something you can copy and paste as a valid payload.

Also, let's be consistent with indentation (e.g. 2 spaces):

{
  "report": [
    {
      "date": "2018-11-28T17:34:45Z",
      "amount": 1234,
      "balance": 12345678,
      "description": "blah"
    }
  ]
}

Applies below as well.

@kinbiko kinbiko merged commit 4c4e4ef into master Nov 28, 2018
@kinbiko kinbiko deleted the api-design branch November 28, 2018 19:35
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