GMIM-353: Add utility method to fetch widget data...#79
GMIM-353: Add utility method to fetch widget data...#79yashdattsawant wants to merge 47 commits intomasterfrom
Conversation
…ue event_timeline.
|
Is there any context on these changes? I'm unable to access the GMI jira project to look at the ticket |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #79 +/- ##
==========================================
- Coverage 22.54% 22.50% -0.05%
==========================================
Files 11 11
Lines 1433 1480 +47
Branches 342 351 +9
==========================================
+ Hits 323 333 +10
- Misses 1110 1147 +37 ☔ View full report in Codecov by Sentry. |
|
@ekauzlarich can you take a look and re-review? |
ekauzlarich
changed the title
ekauzlarich
left a comment
ekauzlarich
left a comment
There was a problem hiding this comment.
could you also add some unit test coverage for the new functionality? doesn't need to be 100% or anything but it's very low right now.
| html_content = self.session.get(url).text | ||
| soup = BeautifulSoup(html_content, "html.parser") | ||
| table = soup.find("table") | ||
| rows = table.find("tbody").find_all("tr") |
There was a problem hiding this comment.
can you simplify via something like rows = soup.select("table tbody tr") ?
There was a problem hiding this comment.
also I'm unfamiliar with what is callable from this SDK or not, is /dev/udf/notebooks available instead of parsing the HTML table? this is prone to breaking if the page is ever updated
There was a problem hiding this comment.
Thanks for the endpoint. It's available. updated the code accordingly.
| async_task_id: str = results.json().get("response").get("task_id") | ||
|
|
||
| results = self.session.get(url + "/" + async_task_id).json() | ||
| time.sleep(10) |
There was a problem hiding this comment.
instead of sleeping manually 10s twice, this should loop until a specified timeout for task completion
|
|
||
| if "machine__source" not in kwargs and "machine__source__in" not in kwargs: | ||
| log.warn("Machine source not specified.") | ||
| return [] |
There was a problem hiding this comment.
where is this function called? also should this return a request error of some sort since it seems like these params are required for it to do anything?
There was a problem hiding this comment.
will take a look at these. Thanks for feedback
There was a problem hiding this comment.
For this function. Yes it's not being used as if now. wanted to have a placholder for manipulating the workspace. we have added the workspace manipulation diff tool. We might not need this. will discuss this with Nate and take the call over this.
|
Update: addressed all the suggestions. working on writing test cases. parallelly working on other tickets. will try to wrap in couple of days |
4 participants
No description provided.