Page MenuHomePhabricator

implement select & html classes
AbandonedPublic

Authored by brooke on Jul 28 2019, 3:57 PM.

Details

Reviewers
chris
Test Plan

code review

Diff Detail

Repository
rSIM Simplex
Branch
dev
Lint
Lint ErrorsExcuse: update
SeverityLocationCodeMessage
Errorsimplex\application\storage\data_management.py:31E128PEP8 E128
Errorsimplex\application\storage\data_management.py:32E128PEP8 E128
Errorsimplex\application\storage\data_management.py:47E222PEP8 E222
Errorsimplex\application\storage\data_management.py:47E303PEP8 E303
Errorsimplex\view\simui\simui_create_html_view.py:17E222PEP8 E222
Errorsimplex\view\simui\simui_select_item_view.py:7E203PEP8 E203
Errorsimplex\view\simui\simui_select_item_view.py:12E203PEP8 E203
Unit
Unit Tests OK
Build Status
Buildable 37
Build 37: arc lint + arc unit

Event Timeline

brooke requested review of this revision.Jul 28 2019, 3:57 PM
brooke created this revision.
brooke updated this revision to Diff 51.Jul 28 2019, 4:05 PM

update after merging

chris requested changes to this revision.Jul 28 2019, 5:11 PM
chris added inline comments.
simplex/application/storage/data_management.py
2–3

I'm leaning towards (ref D15) abstracting the imports a little bit so that they're independent of the actual underlying file structure. In this case, I think the pattern would look like:

simplex/view/__init__.py
from simplex.view.simui.simui_select_item_view import SIMUISelectItemView
from simplex.view.simui.simui_create_html_view import SIMUIMakeHtmlTableView
simplex/__init__.py
from simplex.view import simui as SIMUI
simplex/application/storage/data_management.py
from simplex.SIMUI import SIMUISelectItemView, SIMUIMakeHtmlTableView
3

And from a naming perspective, mind moving from {nav create_html_table_view > simui_table_view} and from {nav SIMUIMakeHtmlTableView` > SIMUITableView?

11

What's the pandas default behavior here? Does it make a deep or shallow copy? I don't remember.

If it's a deep copy, might want to skip this step to conserve resources

30

Style-wise, mind changing to

options = (
    SIMUISelectItemView()
    .set_select_options(['object', 'float', 'integer'])
    .render())

for consistency with elsewhere in the codebase?

37

I don't think we want to be building UI components by hand outside of SIMUI, do we?

simplex/simplex.py
11

Why are you importing seaborne here? Just out of curiosity.

I think we might want to move that import to whatever part of the codebase is actually going to be responsible for drawing visualizations

50

Why are we assuming no header? Will this cause data types to be guessed incorrectly if the data actually do contain a header row?

simplex/view/simui/simui_create_html_view.py
9

Please try to stick to single quotes, don't switch between double and single quotes. Just to keep things clean and simple!

simplex/view/simui/simui_select_item_view.py
22

Will this ever return the selected option as being marked selected? I don't think it will, right?

This revision now requires changes to proceed.Jul 28 2019, 5:11 PM
brooke added inline comments.Jul 30 2019, 8:36 PM
simplex/application/storage/data_management.py
3

will rearrange things accordingly

30

yep agree.

37

Agree. wait for me. I am figuring it out.

simplex/simplex.py
11

I use sns to get iris data when I was debugging. This should be removed.

50

In my csv data, the first row is header. If I do not specify header=None, it will take the first row as header and when I call simui_create_html_view to create the preview table the header will not show.

simplex/view/simui/simui_create_html_view.py
9

Will go through scripts and clean things up

simplex/view/simui/simui_select_item_view.py
22

no, it will not. I am working on that.

brooke abandoned this revision.Aug 7 2019, 5:59 PM