PTV-1810 fix obj input 1 #3320
PTV-1810 fix obj input 1 #3320
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #3320 +/- ##
===========================================
- Coverage 29.22% 5.62% -23.61%
===========================================
Files 495 496 +1
Lines 50482 50486 +4
===========================================
- Hits 14755 2841 -11914
- Misses 35727 47645 +11918
... and 326 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
Codacy is whining about |
| @@ -162,6 +99,31 @@ def _untransform(transform_type, value): | |||
|
|
|||
|
|
|||
| def app_param(p): | |||
| """ | |||
There was a problem hiding this comment.
Added a bit of notes here that weren't exactly intended for this PR, though still helpful.
| @@ -0,0 +1,64 @@ | |||
| import os | |||
There was a problem hiding this comment.
This is the bulk of the change - migrate system_variable and strict_system_variable over here.
Really, these should be separate functions - get_workspace, get_workspace_id, etc. But that's out of scope for this little project. It was originally done with strings to map directly to various app spec requests.
| @@ -0,0 +1,90 @@ | |||
| import os | |||
There was a problem hiding this comment.
Redid the tests here as pytest style, since we're using pytest anyway and it would be nice to migrate there.
| @@ -5,20 +5,19 @@ | |||
| """ | |||
|
|
|||
| import re | |||
|
|
|||
| from .app_util import system_variable | |||
| from .system import system_variable | |||
|
|
|||
| external_tag = "&" | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
The various typing things also weren't exactly intended to pop into this PR, but they're benign and generally helpful.
src/biokbase/narrative/system.py
Outdated
| else: | ||
| return None |
There was a problem hiding this comment.
can just do return None here - no need for final else.
src/biokbase/narrative/system.py
Outdated
| if var == "workspace": | ||
| return os.environ.get("KB_WORKSPACE_ID", None) | ||
| elif var == "workspace_id": | ||
| ws_name = os.environ.get("KB_WORKSPACE_ID", None) |
There was a problem hiding this comment.
Doesn't os.environ.get("blahblahblah") return None by default if it can't find the value?
There was a problem hiding this comment.
Yup. I really didn't modify that code, just copy-pasted. I'll make it better.
There was a problem hiding this comment.
Fixed up. Also swapped BaseException over to Exception for those try/catch blocks. That #TODO in there still holds - we should do some better error trapping there.
|
Kudos, SonarCloud Quality Gate passed!
|
Description of PR purpose/changes
This is the first of probably 3 or 4 PRs related to some fixes for PTV-1810.
The short version is with the recent updates to that bug, input objects are stored in app cells as UPAs and passed to the app framework that way. The problem here is that apps don't always expect UPAs (ALTHOUGH THEY SHOULD). Older ones can expect object names, or references of the form ws_name/obj_name.
The goal of this chain of PRs is to update the Narrative backend code to detect those requirements and parse UPAs back into lesser, worse formats for apps that want them. Backward compatibility and all that.
This will mainly require updating
biokbase.app_util.transform_object_value- an old bit of code that, with the lightest poking, reveals ancient misguided decisions enabled by Python's "anything goes!" flexibility around input types. Again, the goal isn't to fully fix all of that, which would be a very large effort, but to put in an effective patch without creating more chaos. Currently, that function implicitly expects that an object name is provided, and transforms based on that. This update will explicitly expect that any object-ish string (name, reference, reference-path, UPA) is provided, and transform based on the given parameter spec.Sounds simple, but it's less so.
PR strategy:
system_variableandstrict_system_variablefunctions to their own module to avoid what will become circular dependencies in the next PR.Jira Ticket / Issue
Related Jira ticket: https://kbase-jira.atlassian.net/browse/PTV-1810
DATAUP-69 Adds a PR template)Testing Instructions
Dev Checklist:
Updating Version and Release Notes (if applicable)