Page MenuHomePhabricator

updates based on feedbacks from D18
ClosedPublic

Authored by brooke on Aug 4 2019, 5:08 PM.

Details

Summary

fix D18

Test Plan

code review

Diff Detail

Repository
rSIM Simplex
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

brooke requested review of this revision.Aug 4 2019, 5:08 PM
brooke created this revision.
Harbormaster completed remote builds in B39: Diff 55.Aug 4 2019, 5:08 PM
brooke updated this revision to Diff 56.Aug 4 2019, 5:10 PM
  • remove hard coded style
Harbormaster completed remote builds in B40: Diff 56.Aug 4 2019, 5:10 PM
chris added a comment.Aug 4 2019, 7:41 PM

Looking good! Couple minor comments here and there.

Does this supercede D18? Did you want to go ahead and abandon that one, or does this build onto it?

simplex/application/data/data_management.py
2

Do you mind just adding a

# [TODO]: @brooke
# Use __init__ to abstract imports from underlying file hierarchy

comment so we don't lose track of this?

13–15

I think this is still a little redundant. Would

data = self.data.head()

work here?

33

In the future, will this be expanded to all valid Pandas dtypes?

34

And this'll be picked up on the fly in a future iteration?

35

What are your thoughts on setting this to the actual name of the pandas column? That should enforce a uniqueness constraint.

35

This should probably also be changed to set_id, since name is also a valid attribute for an HTML element, but you're actually setting the element id here.

simplex/simplex.py
7–9

Same thing here with the TODO comment

simplex/view/simui/simui_select_item_view.py
9

Just for PEP8 style stuffs, mind removing the space before the : here? I.e., options: list

27

Since this is a relatively short string, I think you can get away with just

select_content = '<select id={}>{}</select>'.format(
    ...)
simplex/view/simui/simui_table_view.py
11

I think indentation here got wonky

brooke updated this revision to Diff 59.Aug 4 2019, 8:06 PM
  • get selected item from dtypes
Harbormaster completed remote builds in B43: Diff 59.Aug 4 2019, 8:06 PM
brooke added inline comments.Aug 7 2019, 5:44 PM
simplex/application/data/data_management.py
2

will do so.

13–15

agree

33

Oh yeah I forgot bool.

34

I am not sure I understand.

35

agree.

35

oh yeah, it should not be hard coded. good idea.

simplex/view/simui/simui_select_item_view.py
9

good catch.

27

agree.

simplex/view/simui/simui_table_view.py
11

will fix it

brooke updated this revision to Diff 60.Aug 7 2019, 5:44 PM
  • update codes based on review feedbacks
Harbormaster completed remote builds in B44: Diff 60.Aug 8 2019, 6:32 PM
chris accepted this revision.Aug 8 2019, 6:36 PM
chris added inline comments.
simplex/application/data/data_management.py
34

You can ignore this comment — you addressed already! Originally, you were setting everything to an integer, but it looks like you're now dynamically grabbing each column's dtype, so we're all good!

simplex/view/simui/simui_table_view.py
22

For consistency, mind renaming this to render()? Just easier to remember if everything has a generic render method than trying to figure out what it is for some specific class or other.

This revision is now accepted and ready to land.Aug 8 2019, 6:36 PM
chris requested changes to this revision.Aug 8 2019, 6:37 PM

Whoops! Sorry, almost forgot: mind fixing the PEP lint issues also?

This revision now requires changes to proceed.Aug 8 2019, 6:37 PM
brooke updated this revision to Diff 61.Aug 11 2019, 1:50 PM
  • fix lint issues
chris added inline comments.Aug 11 2019, 1:54 PM
simplex/templates/manage/data.html
6

Could you keep HTML as double quotes? Otherwise it can just get annoying to escape things when you want to reference Python objects inside of an HTML-quoted field.

simplex/view/simui/simui_table_view.py
22

This is still outstanding

brooke updated this revision to Diff 62.Aug 11 2019, 2:00 PM
  • fix style issues
chris accepted this revision.Aug 11 2019, 2:01 PM
This revision is now accepted and ready to land.Aug 11 2019, 2:01 PM
This revision was automatically updated to reflect the committed changes.