Skip to content

Copy ome.api.* documentation to omero.api.* (5)#4702

Merged
jburel merged 7 commits intoome:developfrom
sbesson:ome_omero_apidoc_5
Jun 20, 2016
Merged

Copy ome.api.* documentation to omero.api.* (5)#4702
jburel merged 7 commits intoome:developfrom
sbesson:ome_omero_apidoc_5

Conversation

@sbesson
Copy link
Copy Markdown
Member

@sbesson sbesson commented Jun 8, 2016

See https://trello.com/c/JQ9AcNa2/77-copy-ome-api-documentation-to-omero-api

Follow-up of #4547, #4569, #4578 and #4605, this PR copies the remaining ome.api.* Javadoc documentation into the slice omero.api.* classes. It also includes a minor fix reported in #4605 (comment). This should be the final set of migration PRs associated with this documentation epic. Thanks to @ximenesuk for his infinite patience.

To review this PR, either build the slice documentation via release-slice2html or use the published CI documentation or OMERO.docs* artifacts. Check the links are all working and the method parameters match.

@sbesson sbesson added the develop label Jun 8, 2016
* the family.
* @param noiseReduction Pass <code>true</code> to turn the
* noise reduction algorithm on,
* <code>false</code> otherwise.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think these need fixing now - as it was in the original docs - but this use of 'otherwise' for a parameter is not quite right. It's fine for a return value which is dependent on something but here it is simply 'The value passed to the noise reduction algorithm' or 'The boolean value...' This occurs a good number of times in this file where booleans are passed in.

@ximenesuk
Copy link
Copy Markdown
Contributor

This all looks fine, rendering and links look fine. The one comment is more for the future if you want to get this merged now.

@jburel
Copy link
Copy Markdown
Member

jburel commented Jun 9, 2016

Creates a instance of the rendering engine => an

@jburel
Copy link
Copy Markdown
Member

jburel commented Jun 9, 2016

for getColorModel and setColorModel I will add @see #getAvailableColorModels

@jburel
Copy link
Copy Markdown
Member

jburel commented Jun 9, 2016

No doc for setChannelLookupTable and getChannelLookupTable
Plan to be added in another PR?

@jburel
Copy link
Copy Markdown
Member

jburel commented Jun 9, 2016

Search.ice: missing double-quote e.g."by*". This will create issue with ice 3.6 due to a parsing error.

@sbesson
Copy link
Copy Markdown
Member Author

sbesson commented Jun 9, 2016

@jburel: I do not have any plan to open a PR adding extra content yet. Instead, the assumption was that the documentation will be reviewed in both places as part of future feature work.
Good catch about the double-quote but should our own Ice 3.6 builds have caught this in the first place? I will push commit for the minor fixes.

@jburel
Copy link
Copy Markdown
Member

jburel commented Jun 10, 2016

If I remember correctly, the problem showed up when I started the server and the service was used.

@jburel
Copy link
Copy Markdown
Member

jburel commented Jun 10, 2016

I should have mentioned:
the argument is "cell", one return value might have as its * textValue: "cellular" while another has "cellularize".

@jburel
Copy link
Copy Markdown
Member

jburel commented Jun 10, 2016

Looking at the unimplemented LUT methods, now I am wondering if it will not be better to pass an ID instead of a string for the LUT.
this is outside the scope of the PR so the doc can be left blank for now
we can add doc when we start working on LUT

@sbesson
Copy link
Copy Markdown
Member Author

sbesson commented Jun 10, 2016

Briefly discussed with @jburel and @ximenesuk. I will push a final commit here that reviews all worrying usages of double-quote in the Slice documentation (with regard to Ice 3.6.2) and replaces them using other types of markup. One follow-up PR here might be to codify all the stylistic decisions made as part of this set of PRs into a guideline document.

@sbesson
Copy link
Copy Markdown
Member Author

sbesson commented Jun 10, 2016

Pushed. Ideally the output of

$ git grep '\"' -- components/blitz/resources/omero/api/ | grep -v "\[\"" | grep -v href

should be reviewed to make sure we have not left any unsafe double quote.

@ximenesuk
Copy link
Copy Markdown
Contributor

With this PR the output looks fine.

colin$ git grep '\"' -- components/blitz/resources/omero/api/ | grep -v "\[\"" | grep -v href
components/blitz/resources/omero/api/IContainer.ice:                 *            Timestamp.valueOf("YYYY-MM-DD hh:mm:ss.ms");
components/blitz/resources/omero/api/IContainer.ice:                 *              Timestamp.valueOf("YYYY-MM-DD hh:mm:ss.ms").
components/blitz/resources/omero/api/IScript.ice:                 *     path = script.path.val\[1:\] # First symbol is a "/"
components/blitz/resources/omero/api/IScript.ice:                 *     path = path.replace("/",".")
components/blitz/resources/omero/api/IScript.ice:                 *     print "Possible import: %s" % path
components/blitz/resources/omero/api/MetadataStore.ice:        const string METADATASTORE = "omero.api.MetadataStore";

I assume it builds okay?

Probably not in the scope of this PR but:

https://ci.openmicroscopy.org/view/DEV/job/OMERO-DEV-merge-build/ICE=3.5,jdk=8_LATEST,label=octopus/javadoc/slice2html/omero/api/ITimeline.html#ITimeline

is a bit of a mess. This wasn't one of the files dealt with in this series of PRs.

@sbesson
Copy link
Copy Markdown
Member Author

sbesson commented Jun 17, 2016

I can try and tidy up ITimeline in a follow-up PR. From an previous discussion with @jburel, the line that should be the most worrisome is the one with a trailing " symbol i.e.

components/blitz/resources/omero/api/IScript.ice:                 *     path = script.path.val\[1:\] # First symbol is a "/"

If we want to be on the safe side, I could drop the double quote (or add a trailing dot).

@jburel
Copy link
Copy Markdown
Member

jburel commented Jun 17, 2016

we can clean ITimeline in another PR, since this PR will probably now conflict with the work I am doing on LUT support (https://github.com/jburel/openmicroscopy/tree/lut-repo)
I will remove the ".

@sbesson
Copy link
Copy Markdown
Member Author

sbesson commented Jun 17, 2016

@jburel: see b7f9f49

@jburel
Copy link
Copy Markdown
Member

jburel commented Jun 20, 2016

@sbesson: thanks, Looks good. @ximenesuk happy with the status of this PR.
If no objection, I will merge it

@ximenesuk
Copy link
Copy Markdown
Contributor

If slice is happy I'm happy!

@jburel jburel merged commit 5839b0b into ome:develop Jun 20, 2016
@sbesson sbesson deleted the ome_omero_apidoc_5 branch June 20, 2016 12:44
@sbesson sbesson added this to the 5.3.0 milestone Sep 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants