Uh oh!
There was an error while loading. Please reload this page.
Item #6862: provenance loadRun API - #1000
Merged
Merged
Conversation
labkey-kevink
commented
Mar 27, 2020
Contributor
- refactor linage API to use same serialization as loadRun API
- add serialization options to include properties, run steps, and input/outputs
- include provenance in loadRun and XAR export
- include provenance properties in the loadRun response - share serialization of experiment objects in lineage API and loadRun API - add Identifiable.detailsURL
- add QueryRowReference to Identifiable and include it when in lineage and loadRun response - optionally include run steps in LABKEY.Experiment.loadRun() response - optionally include provenance map in LABKEY.Experiment.loadRun() response
labkey-jeckels
approved these changes
Mar 27, 2020
labkey-jeckels
left a comment
Contributor
There was a problem hiding this comment.
Smaller suggestions, no further review required.
| } | ||
| public interface LsidHandler | ||
| public interface LsidHandler<O extends Identifiable> |
Contributor
There was a problem hiding this comment.
Consider using something other than "O" - I find that reading the diff it keeps looking like "0" (aka zero) when it stands all alone.
| return json; | ||
| } | ||
| // Serialize only the base properties -- does not include object properties |
Contributor
There was a problem hiding this comment.
Can you better summarize the difference between "base" and "standard" properties here? Especially muddled because the first comment in this method says "standard properties"
| import static org.labkey.api.util.PageFlowUtil.encode; | ||
| /** | ||
| * Reference a single row within a table by it's query coordinates: container, schemaName, queryName, and a set of pk filters. |
Contributor
There was a problem hiding this comment.
Suggested change
| * Referenceasinglerowwithinatablebyit's query coordinates: container, schemaName, queryName, and a set of pk filters. | |
| * Referenceasinglerowwithinatablebyitsquerycoordinates: container, schemaName, queryName, andasetofpkfilters. |
| * @param {Boolean} config.includeInputsAndOutputs Include run and step inputs and outputs. | ||
| * @param {Boolean} config.includeRunSteps Include run steps. | ||
| * @param {function} config.success The function to call when the function finishes successfully. | ||
| * This function will be called with a the parameters: |
Contributor
There was a problem hiding this comment.
Suggested change
| *Thisfunctionwillbecalledwithatheparameters: | |
| *Thisfunctionwillbecalledwiththeparameters: | |
labkey-nicka
approved these changes
Mar 27, 2020
labkey-nicka
left a comment
Contributor
There was a problem hiding this comment.
Looks really good. Reviewed what is being returned in some of the scenarios I'm looking at for the lineage graph replacement and I didn't find any discrepancies in my spot checks.
A couple of minor nits.
| public static JSONObject serializeStandardProperties(@NotNull ExpObject object, @Nullable List<? extends DomainProperty> properties, @NotNull Settings settings) | ||
| { | ||
| JSONObject jsonObject = serializeStandardProperties(object); | ||
| JSONObject jsonObject; |
Contributor
There was a problem hiding this comment.
nit: Can hoist serializeBaseProperties(object) out of conditionals.
JSONObjectjsonObject = serializeBaseProperties(object);
if (settings.isIncludeProperties())
{
JSONObjectpropertiesObject = serializeCustomProperties(object, properties, settings);
if (!propertiesObject.isEmpty())
jsonObject.put(PROPERTIES, propertiesObject);
}
returnjsonObject;| import org.labkey.api.exp.Identifiable; | ||
| import org.labkey.api.exp.query.SamplesSchema; | ||
| import org.labkey.api.query.SchemaKey; | ||
| import org.labkey.api.security.User; |
Contributor
There was a problem hiding this comment.
nit: ExpLineage.java and QueryRowReference.java have unused imports.
This was referenced Jul 30, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.