Feedback for class structure

[xiekun]

• gslides version: 0.1.0
• Python version: 3.8.8
• Operating System: Windows 10

Description

Play around with the package and offer opinions

What I Did

• Run all cells in usage.ipynb, troubleshoot any errors
• Look through slides.py and addchart.py

Issues Noted:

• in usage.ipynb, I was able to directly run creds = flow.run_local_server() without specifying a port
• iris = datasets.load_iris(as_frame = True)['data'] missing dependency on sklearn, maybe embed a flat file directly in repo?

Feedback:

I think there is some inconsistency in class name that will hinder user when building a mental model, Currently it is:

 - Level 1: CreatePresentation
 - Level 2: CreateSlide
 - Level 3: Chart (Why not CreateChart)
 - Level 4: Series (Why not CreateSeries)

It can be consistent this way:

 - Level 1: Presentation
 - Level 2: Slide
 - Level 3: Chart/Table/Image
 - Level 4: Series

Expectation: When user see CreatePresentation, they intuitively will expect CreateChart and vice versa.

The method of initialize is also not consistent, currently it is:

new_chart = Chart(argument that describe the chart)
new_series = Scatter(argument that describe the series, except the type)
new_presentation = CreatePresentation(argument that describe presentation)

I think rest of approach should be consistent to this, i.e

new_chart = Chart(argument that describe the chart)
new_series = Series(arguments,...,type='Scatter')
new_presentation = Presentation(argument that describe presentation)

and adjusting each object via add/remove method with consistent pattern i.e.

new_presentation = Presentation(Slides = [slide1,slide2])  #Create at initilization
new_presentation.addSlide(slide3) #now the presentation has 3 slidess
new_presentation.rmSlide(slide1) #now the presentation has 2 slides but different from before

Specifically on charts, a nice feature in popular packages is FacetGrid , and this is not doable in Excel/Sheets manually. This will become an attractive feature but requires a bit of planning in class structure:

• i.e where do user specify arguments (Slide level vs Chart Level)
  • How do you implement a map() function (e.g. change the second series of all charts in a slide)

A styler object that can be applied on presentation/slide/chart level, this is already covered in your feature planning

You can make argument name more mainstream, it will reduce cognitive overhead for new users:

• For existing python users, rather than x_columns,y_columns,series, use name x,y,hue/color, people with matplotlib/seaborn/plotly will pick it up easier
• For google sheet users, name [x_columns,y_columns] to [x_axis, series], this is familiar existing users of sheets who do their work manually
• I think the above two are contradictory, I'd stick with the former

These are all opinion, I didn't quite get how Gsuite api works or if any of these is possible.

[michael-gracie]

For existing python users, rather than x_columns,y_columns,series, use name x,y,hue/color, people with matplotlib/seaborn/plotly will pick it up easier

I agree with this comment, will implement

[michael-gracie]

I think there is some inconsistency in class name that will hinder user when building a mental model

I agree, let's Option 1 classes that represent actions and Option 2 class that represent objects. To your point at the moment I have a mix of 2. I'll walk through my thinking using the SheetsFrame parent class as an example. I do agree that I could move to an object based approach but want to talk through it.

Action Approach

I originally defined the classes as actions because the initialization of a GetFrame class and CreateFrame class needed to be different. For GetFrame the user needs to provide cells that would bound the rows/columns to retrieve, for CreateFrame a pandas dataframe is required.

Object approach

If I were to refactor to take an object based approach perhaps I would do the following:

- class: Frame
  - method: __init__ --> initializes empty object
  - method: create --> runs CreateFrame(args).execute()
  - method: get --> runs GetFrame(args.execute())
  - property: data --> only returns `data` if either `create` or `get` has been executed

I think this structure could be feasible for a number of different objects

- class: Chart
  - method: __init__ --> initializes empty object
  - method: create --> runs existing class Chart(args).execute()
  - method: get --> runs a method to get an existing chart in sheets
  - property: chart_id --> only returns `chart_id` if either `create` or `get` has been executed

- class: Presentation
  - method: __init__ --> initializes empty object
  - method: create --> runs CreatePresentation(args).execute()
  - method: get --> runs a method to get an existing presentation and get all the slides contained in the presentation
  - method: add_slide --> runs CreateSlide(args).execute()
  - method: rm_slide --> runs a method to remove a slide
  - method: update_slide --> runs a method to update data on a slide (would need to think about this more)
  - property: presentation_id --> only returns a `presentation_id` if either `create` or `get` has been executed
  - property: sld_id --> returns a list of `sld_id`  if either `create` or `get` has been executed

- class: SpreadSheet
  - method: __init__ --> initializes empty object
  - method: create --> runs CreateSpreadsheet(args).execute()
  - method: get --> runs a method to get an existing spreadsheet and get all the sheets contained in the presentation
  - method: add_sheet --> runs CreateSlide(args).execute()
  - method: rm_sheet --> runs a method to remove a slide
  - property: spreadsheet_id --> only returns a `spreadsheet_id` if either `create` or `get` has been executed
  - property: sheet_id --> returns a list of `sheet_id`  if either `create` or `get` has been executed

[michael-gracie]

new_presentation = Presentation(Slides = [slide1,slide2]) #Create at initilization

I'm thinking of not creating a Slide object for simplicity. Like sheets I think slides have to belong to their parent object (presentations) and can't be initialized on their own

[michael-gracie]

new_series = Scatter(argument that describe the series, except the type)

This was something I spent a lot of time thinking about. Based on the type of series only certain parameters can be set (e.g. a column chart can't have a line_width). I enforced this through creating separate classes with unique initializations.

Perhaps is an alternative is like so:

- class: Series
  - method: __init__ --> initializes empty series object
  - method: scatter --> initializes scatterplot
  - method: line --> initialized line
  .
  .
  .

[jfyu]

Before I test anything and since there is a refractoring on the horizon, if you are considering to do

- class: Frame
  - method: __init__ --> initializes empty object
  - method: create --> runs CreateFrame(args).execute()
  - method: get --> runs GetFrame(args.execute())
  - property: data --> only returns `data` if either `create` or `get` has been executed

It might be good to create a super class with init/create/get and for all sub classes to inherit to be more DRY.

[michael-gracie]

Before I test anything and since there is a refractoring on the horizon, if you are considering to do

- class: Frame
  - method: __init__ --> initializes empty object
  - method: create --> runs CreateFrame(args).execute()
  - method: get --> runs GetFrame(args.execute())
  - property: data --> only returns `data` if either `create` or `get` has been executed

It might be good to create a super class with init/create/get and for all sub classes to inherit to be more DRY.

Do you prefer the action based or object based classes? I haven't completed decided yet to do the refactoring

[jfyu]

object :)

[michael-gracie]

@xiekun @jfyu just pushed the refactor to main branch 😄 . Plus I added table functionality. For some of the other feedback like logging I need to create issues and resolve later