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

changed the spec issues-api #23

Merged
merged 13 commits into from
Sep 6, 2018
Merged

changed the spec issues-api #23

merged 13 commits into from
Sep 6, 2018

Conversation

achebanets
Copy link
Contributor

No description provided.

@achebanets achebanets requested a review from hchaps as a code owner August 27, 2018 15:33
description: >-
Currently the only allowed issue states are `OPENED` and `RESOLVED` but
Smartling may introduce additional states in future. This request
returns all current allowed states.
tags:
- Issues
- Dictionaries
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is how it groups the API's, so it should still be under Issues i believe.

in: path
required: true
type: string
description: ''
Copy link
Contributor

Choose a reason for hiding this comment

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

description should be provided

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

in: path
required: true
type: string
description: ''
Copy link
Contributor

Choose a reason for hiding this comment

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

please add description (this applies to all params, fields, etc)

@@ -2545,7 +2610,7 @@ paths:
as needed before attempting to render it. Maximum length is 4000
characters.
tags:
- Issues
- Comments
Copy link
Contributor

Choose a reason for hiding this comment

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

i believe it should be issues still

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 Issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@jones-smartling jones-smartling left a comment

Choose a reason for hiding this comment

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

A number of descriptions are not provided and tags look off to me.

get:
summary: List all issue types
summary: Get Issue Types (DEPRECATED)
description: >-
This request returns all current issue types. Smartling may occasionally
Copy link
Contributor

@jones-smartling jones-smartling Aug 28, 2018

Choose a reason for hiding this comment

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

since it's deprecated, I think the description should say what should be used instead

description: >-
This request returns all current issue types. Smartling may occasionally
add or change the list of allowed issue types.
tags:
- Issues
deprecated: true
Copy link
Contributor

Choose a reason for hiding this comment

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

didnt know they had this, good to know

@jones-smartling
Copy link
Contributor

jones-smartling commented Aug 28, 2018

issueapidocs

The tags still need to be changed

description: >-
Returns a list of issues matching specified filter options. You can
filter based on the date issues were created, target languages, strings,
issue types and states and the user who opened the issue. Unless
otherwise specified, request will return a maximum of 30 results. All
parameters are optional.
tags:
- Issues
- Search
Copy link
Contributor

Choose a reason for hiding this comment

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

should be Issues

tags:
- Issues
- Watchers
Copy link
Contributor

Choose a reason for hiding this comment

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

should be issues

Copy link
Contributor

@jones-smartling jones-smartling left a comment

Choose a reason for hiding this comment

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

further changes needed still, see comments

@jones-smartling
Copy link
Contributor

I see the response block is not showing up on the rendered page, that needs to be addressed before its merged

get:
summary: List all issue types
summary: Get Issue Types (DEPRECATED)
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont think the first API listed should be a deprecated API. This should be moved to the bottom of the APi's for Issues

in: query
required: false
type: string
description: Unique identifier of corresponding account
Copy link
Collaborator

Choose a reason for hiding this comment

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

The description is off

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

required: false
default: 100
type: number
description: limit
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jones-smartling @hchaps, seems like various apis use different description for limit and offset parameters. Do we want them to be consistent across all apis?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might need @hchaps to help standardize the language here, but for now its ok. We wont get consistent across defaults and such and that is ok. My 2 cents.

@@ -3008,51 +3081,33 @@ paths:
in: path
required: true
type: string
description: Unique identifier of corresponding account
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra space before Unique

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -3065,22 +3120,24 @@ paths:
in: path
required: true
type: string
description: ''
description: Unique identifier of corresponding account
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra space before Unique

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

- name: watcherUid
in: path
required: true
type: string
description: ''
description: Unique identifier of corresponding watcher
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra space before Unique

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

- name: issueUid
in: path
required: true
type: string
description: ''
description: Unique identifier of corresponding issue
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra space before Unique

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done for all places

data:
type: object
properties:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is possible to extract issue and watcher response properties into a separate item and to reuse it wherever needed? Like for get/find/create issues responses?
See

$ref: '#/definitions/ListJobsResponseItem'
as an example

@achebanets achebanets merged commit e9d9df6 into master Sep 6, 2018
@achebanets achebanets deleted the update-issues-api-section branch September 24, 2018 11:22
rsukharnyk-smartling added a commit that referenced this pull request Apr 19, 2023
 - #23 - wrong reference fix
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