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

[API] Adding endpoint for project's images #3369

Merged
merged 11 commits into from
Feb 19, 2018

Conversation

xlecours
Copy link
Contributor

@xlecours xlecours commented Jan 3, 2018

This pull request adds an endpoint to provide image file links. It support a single parameter: since
Additional data for each image would help for filtering.

There might be a need to add QC status for them as well.

  • Doc

adding rewrite rules

adding docs
@xlecours xlecours added Area: API PR or issue related to the API Category: Feature PR or issue that aims to introduce a new feature [branch] minor labels Jan 3, 2018
@xlecours xlecours added this to the 19.1 milestone Jan 3, 2018
@xlecours xlecours mentioned this pull request Jan 3, 2018
1 task
@@ -1,4 +1,4 @@
# Loris API - v0.0.2
# Loris API - v0.0.3-dev
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

review of the doc

@@ -139,6 +139,29 @@ The body of the request to /projects/$ProjectName will be an entity of the form:
}
```

```
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing a section header

GET /projects/$ProjectName/images/
```

Will return a JSON object of the form
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing a colon at the end

@@ -139,6 +139,29 @@ The body of the request to /projects/$ProjectName will be an entity of the form:
}
```

```
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing a section header

{
"candidate": "123456",
"visit": "V1",
"scan_type": Acquisition protocol ex :"t2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

the reason for using "js" as the type for the block instead of json elsewhere is so that you can use comments. So you can do smething like "scan_type": "t2", /* acquisition protocol */ and have it highlighted properly in the markdown.

The capitalization of this is also inconsistent with the rest of the endpoints (or at least the ones showing up as context in this diff) where the first letter is capitalized, and CapitalizedWordCase is used, not underscores.

"candidate": "123456",
"visit": "V1",
"scan_type": Acquisition protocol ex :"t2",
"link": URL relative to this API ex: "\/candidates\/300022\/V1\/images\/loris-MRI_123456_V1_t2_001.mnc",
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment about using JS comments as above.

]
}
```
It is possible to provide a parameter named `since` where the value need to be a date or datetime ex: 2016-08-09 or 2016-08-09 10:00:00 or 2016-08-09T10:00:00
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should specify a GET parameter to avoid ambiguity of whether POST parameters are accepted.

The example values need quotation marks, and should probably only have one option to simplify things and avoid ambiguity. It'd probably be best to take an existing standardized date format that's close enough to the last option ( RFC3339? There's a built in constant in PHP for it, too, which should make it easy to parse: http://php.net/manual/en/class.datetime.php#datetime.constants.rfc3339) and reference that.

}
```
It is possible to provide a parameter named `since` where the value need to be a date or datetime ex: 2016-08-09 or 2016-08-09 10:00:00 or 2016-08-09T10:00:00
If the timezone is provided, it will be ignored.
Copy link
Collaborator

@driusan driusan Jan 3, 2018

Choose a reason for hiding this comment

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

That (ignoring timezone) seems like a bad idea.

@xlecours xlecours changed the title adding endpoint [API] Adding endpoint for project's images Jan 3, 2018
@xlecours
Copy link
Contributor Author

xlecours commented Jan 3, 2018

@driusan Thanks for your comments. This PR seems much better now

@kongtiaowang kongtiaowang added the Passed manual tests PR has been successfully tested by at least one peer label Jan 11, 2018
@kongtiaowang
Copy link
Contributor

screen shot 2018-01-11 at 2 52 31 pm

Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

I still think it should refer to a specific date format rather than referring to PHP documentation and saying "date format supported by PHP", but that's not a blocker.

@driusan driusan merged commit 3861355 into aces:minor Feb 19, 2018
zaliqarosli pushed a commit to zaliqarosli/Loris that referenced this pull request May 2, 2018
This adds an endpoint to provide image file links. It support a single GET parameter: "?since=", which takes a date as a parameter and returns the images since that date.
@xlecours xlecours deleted the api_images_since branch August 4, 2021 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: API PR or issue related to the API Category: Feature PR or issue that aims to introduce a new feature Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants