Problems generating definitions for `react-table`

[baronfel]

Hi, I ran into a few issues when trying to generate bindings for react-table from DefinitelyTyped using the online ts2fable.

From the above link I generate the following F# source:

F# autogen code for react-table

```fsharp module Fable.Import.React.Table2 // ts2fable 0.6.1 open System open Fable.Core open Fable.Import.JS type [] IExports = abstract ReactTableDefaults: TableProps abstract ReactTable: ReactTableStatic type [] ReactTableFunction = [] abstract Invoke: ?value: obj option -> unit type AccessorFunction = AccessorFunction type [] AccessorFunction<'D> = [] abstract Invoke: row: 'D -> obj option type Accessor = Accessor type Accessor<'D> = U3, AccessorFunction<'D>> [] module Accessor = let ofString v: Accessor<'D> = v |> U3.Case1 let isString (v: Accessor<'D>) = match v with U3.Case1 _ -> true | _ -> false let asString (v: Accessor<'D>) = match v with U3.Case1 o -> Some o | _ -> None let ofStringArray v: Accessor<'D> = v |> U3.Case2 let isStringArray (v: Accessor<'D>) = match v with U3.Case2 _ -> true | _ -> false let asStringArray (v: Accessor<'D>) = match v with U3.Case2 o -> Some o | _ -> None let ofAccessorFunction v: Accessor<'D> = v |> U3.Case3 let isAccessorFunction (v: Accessor<'D>) = match v with U3.Case3 _ -> true | _ -> false let asAccessorFunction (v: Accessor<'D>) = match v with U3.Case3 o -> Some o | _ -> None type [] Aggregator = [] abstract Invoke: values: obj option * rows: obj option -> obj option type TableCellRenderer = U2<(CellInfo -> obj option -> React.ReactNode), React.ReactNode> [] module TableCellRenderer = let ofCase1 v: TableCellRenderer = v |> U2.Case1 let isCase1 (v: TableCellRenderer) = match v with U2.Case1 _ -> true | _ -> false let asCase1 (v: TableCellRenderer) = match v with U2.Case1 o -> Some o | _ -> None let ofReact.ReactNode v: TableCellRenderer = v |> U2.Case2 let isReact.ReactNode (v: TableCellRenderer) = match v with U2.Case2 _ -> true | _ -> false let asReact.ReactNode (v: TableCellRenderer) = match v with U2.Case2 o -> Some o | _ -> None type [] FilterRender = [] abstract Invoke: ``params``: FilterRenderInvokeParams -> React.ReactElement type [] FilterRenderInvokeParams = abstract column: Column with get, set abstract filter: obj option with get, set abstract onChange: ReactTableFunction with get, set abstract key: string option with get, set type PivotRenderer = U4<(CellInfo -> React.ReactNode), (unit -> obj option), string, React.ReactNode> [] module PivotRenderer = let ofCase1 v: PivotRenderer = v |> U4.Case1 let isCase1 (v: PivotRenderer) = match v with U4.Case1 _ -> true | _ -> false let asCase1 (v: PivotRenderer) = match v with U4.Case1 o -> Some o | _ -> None let ofCase2 v: PivotRenderer = v |> U4.Case2 let isCase2 (v: PivotRenderer) = match v with U4.Case2 _ -> true | _ -> false let asCase2 (v: PivotRenderer) = match v with U4.Case2 o -> Some o | _ -> None let ofString v: PivotRenderer = v |> U4.Case3 let isString (v: PivotRenderer) = match v with U4.Case3 _ -> true | _ -> false let asString (v: PivotRenderer) = match v with U4.Case3 o -> Some o | _ -> None let ofReact.ReactNode v: PivotRenderer = v |> U4.Case4 let isReact.ReactNode (v: PivotRenderer) = match v with U4.Case4 _ -> true | _ -> false let asReact.ReactNode (v: PivotRenderer) = match v with U4.Case4 o -> Some o | _ -> None type [] ComponentPropsGetter0 = [] abstract Invoke: finalState: obj option * rowInfo: obj * column: obj * ?instance: obj option -> obj option type [] ComponentPropsGetterR = [] abstract Invoke: finalState: obj option * ?rowInfo: RowInfo * ?column: obj * ?instance: obj option -> obj option type [] ComponentPropsGetterC = [] abstract Invoke: finalState: obj option * ?rowInfo: obj * ?column: Column * ?instance: obj option -> obj option type [] ComponentPropsGetterRC = [] abstract Invoke: finalState: obj option * ?rowInfo: RowInfo * ?column: Column * ?instance: obj option -> obj option type [] DefaultFilterFunction = [] abstract Invoke: filter: Filter * row: obj option * column: obj option -> bool type [] FilterFunction = [] abstract Invoke: filter: Filter * rows: ResizeArray * column: obj option -> ResizeArray type [] SubComponentFunction = [] abstract Invoke: rowInfo: RowInfo -> React.ReactNode type [] PageChangeFunction = [] abstract Invoke: page: float -> unit type [] PageSizeChangeFunction = [] abstract Invoke: newPageSize: float * newPage: float -> unit type [] SortedChangeFunction = [] abstract Invoke: newSorted: ResizeArray * column: obj option * additive: bool -> unit type [] FilteredChangeFunction = [] abstract Invoke: newFiltering: ResizeArray * column: obj option * value: obj option -> unit type [] ExpandedChangeFunction = [] abstract Invoke: column: obj option * ``event``: obj option * isTouch: bool -> unit type [] ResizedChangeFunction = [] abstract Invoke: newResized: ResizeArray * ``event``: obj option -> unit type [] SortFunction = [] abstract Invoke: a: obj option * b: obj option * desc: obj option -> float type [] Resize = abstract id: string with get, set abstract value: obj option with get, set type [] Filter = abstract id: string with get, set abstract value: obj option with get, set abstract pivotId: string option with get, set /// NOTE: to many configuration ways (only true values are confusing) type [] SortingRule = abstract id: string with get, set abstract sort: U2 option with get, set abstract asc: obj option with get, set abstract desc: obj option with get, set type TableProps<'ResolvedData> = TableProps type TableProps = TableProps type [] TableProps<'D, 'ResolvedData> = inherit obj inherit obj inherit obj inherit obj inherit obj inherit obj /// Default: [] abstract data: ResizeArray<'D> with get, set abstract resolveData: (ResizeArray<'D> -> ResizeArray<'ResolvedData>) option with get, set /// Default: false abstract loading: bool with get, set /// Default: true abstract showPagination: bool with get, set /// Default: false abstract showPaginationTop: bool with get, set /// Default: true abstract showPaginationBottom: bool with get, set /// Default: false abstract manual: bool with get, set /// Default: true abstract multiSort: bool with get, set /// Default: true abstract showPageSizeOptions: bool with get, set /// Default: [5, 10, 20, 25, 50, 100] abstract pageSizeOptions: ResizeArray with get, set /// Default: 20 abstract defaultPageSize: float with get, set /// Default: undefined /// Otherwise take value from 'pageSize' if defined abstract minRows: float option with get, set /// Default: true abstract showPageJump: bool with get, set /// Default: true abstract sortable: bool with get, set /// Default: true abstract collapseOnSortingChange: bool with get, set /// Default: true abstract collapseOnPageChange: bool with get, set /// Default: true abstract collapseOnDataChange: bool with get, set /// Default: false abstract freezeWhenExpanded: bool with get, set /// Default: [] abstract defaultSorting: ResizeArray with get, set /// Default: false abstract showFilters: bool with get, set /// Default: [] abstract defaultFiltering: ResizeArray with get, set /// Default: ... abstract defaultFilterMethod: DefaultFilterFunction with get, set /// Default: ... abstract defaultSortMethod: SortFunction with get, set /// Default: true abstract resizable: bool with get, set /// Default: false abstract filterable: bool with get, set /// Default: [] abstract defaultResizing: ResizeArray with get, set /// Default: false abstract defaultSortDesc: bool with get, set /// Default: [] abstract defaultSorted: ResizeArray with get, set /// Default: [] abstract defaultFiltered: ResizeArray with get, set /// Default: [] abstract defaultResized: ResizeArray with get, set /// Default: {} abstract defaultExpanded: obj with get, set /// On change. abstract onChange: ReactTableFunction with get, set /// Default: string /// Adding a -striped className to ReactTable will slightly color odd numbered rows for legibility /// Adding a -highlight className to ReactTable will highlight any row as you hover over it abstract className: string with get, set /// Default: {} abstract style: obj with get, set /// Global Column Defaults abstract column: obj with get, set /// Array of all Available Columns abstract columns: Array> option with get, set /// Expander defaults. abstract expanderDefaults: obj with get, set /// Privot defaults. abstract pivotDefaults: obj with get, set /// The content rendered inside of a padding row abstract PadRowComponent: (unit -> React.ReactNode) with get, set /// Server-side callbacks abstract onFetchData: (obj option -> obj option -> unit) with get, set /// Control callback for functional rendering abstract children: (FinalState<'ResolvedData> -> (unit -> React.ReactElement) -> Instance<'ResolvedData> -> React.ReactNode) with get, set type [] ControlledStateOverrideProps = /// Default: undefined abstract page: float option with get, set /// Default: undefined abstract pageSize: float option with get, set /// Default: undefined abstract pages: float option with get, set /// Default: undefined abstract sorting: float with get, set /// Default: [] abstract sorted: ResizeArray with get, set /// Default: [] abstract filtered: ResizeArray with get, set /// Default: [] abstract resized: ResizeArray with get, set /// Default: {} abstract expanded: obj with get, set /// Sub component abstract SubComponent: SubComponentFunction with get, set type [] PivotingProps = /// Default: undefined abstract pivotBy: ResizeArray with get, set /// Default: 200 abstract pivotColumnWidth: float with get, set /// Default: _pivotVal abstract pivotValKey: string with get, set /// Default: _pivotID abstract pivotIDKey: string with get, set /// Default: _subRows abstract subRowsKey: string with get, set /// Default: _aggregated abstract aggregatedKey: string with get, set /// Default: _nestingLevel abstract nestingLevelKey: string with get, set /// Default: _original abstract originalKey: string with get, set /// Default: _index abstract indexKey: string with get, set /// Default: _groupedByPivot abstract groupedByPivotKey: string with get, set /// Default: {} - Pivoting State Overrides (see Fully Controlled Component section) abstract expandedRows: ExpandedRows with get, set /// Default: ??? - Pivoting State Callbacks abstract onExpandRow: ReactTableFunction with get, set type [] ExpandedRows = [] abstract Item: idx: float -> U2 with get, set type [] DerivedDataObject = abstract _index: float with get, set abstract _nestingLevel: float with get, set abstract _subRows: obj option with get, set abstract _original: obj option with get, set [] abstract Item: p: string -> obj option with get, set type [] ControlledStateCallbackProps = /// Called when the page index is changed by the user abstract onPageChange: PageChangeFunction with get, set /// Called when the pageSize is changed by the user. The resolve page is also sent /// to maintain approximate position in the data abstract onPageSizeChange: PageSizeChangeFunction with get, set /// Called when a sortable column header is clicked with the column itself and if /// the shiftkey was held. If the column is a pivoted column, `column` will be an array of columns abstract onSortedChange: SortedChangeFunction with get, set /// Called when a user enters a value into a filter input field or the value passed /// to the onFiltersChange handler by the Filter option. abstract onFilteredChange: FilteredChangeFunction with get, set /// Called when an expander is clicked. Use this to manage `expanded` abstract onExpandedChange: ExpandedChangeFunction with get, set /// Called when a user clicks on a resizing component (the right edge of a column header) abstract onResizedChange: ResizedChangeFunction with get, set type [] ComponentDecoratorProps = abstract getProps: U3 with get, set abstract getTableProps: ComponentPropsGetter0 with get, set abstract getTheadGroupProps: ComponentPropsGetter0 with get, set abstract getTheadGroupTrProps: ComponentPropsGetter0 with get, set abstract getTheadGroupThProps: ComponentPropsGetterC with get, set abstract getTheadProps: ComponentPropsGetter0 with get, set abstract getTheadTrProps: ComponentPropsGetter0 with get, set abstract getTheadThProps: ComponentPropsGetterC with get, set abstract getTheadFilterProps: ComponentPropsGetter0 with get, set abstract getTheadFilterTrProps: ComponentPropsGetter0 with get, set abstract getTheadFilterThProps: ComponentPropsGetterC with get, set abstract getTbodyProps: ComponentPropsGetter0 with get, set abstract getTrGroupProps: U2 with get, set abstract getTrProps: U2 with get, set abstract getTdProps: U2 with get, set abstract getTfootProps: ComponentPropsGetter0 with get, set abstract getTfootTrProps: ComponentPropsGetter0 with get, set abstract getPaginationProps: ComponentPropsGetter0 with get, set abstract getLoadingProps: ComponentPropsGetter0 with get, set abstract getNoDataProps: ComponentPropsGetter0 with get, set abstract getResizerProps: ComponentPropsGetter0 with get, set type [] ComponentProps = abstract TableComponent: React.ReactType with get, set abstract TheadComponent: React.ReactType with get, set abstract TbodyComponent: React.ReactType with get, set abstract TrGroupComponent: React.ReactType with get, set abstract TrComponent: React.ReactType with get, set abstract ThComponent: React.ReactType with get, set abstract TdComponent: React.ReactType with get, set abstract TfootComponent: React.ReactType with get, set abstract ExpanderComponent: React.ReactType with get, set abstract AggregatedComponent: React.ReactType with get, set abstract PivotValueComponent: React.ReactType with get, set abstract PivotComponent: React.ReactType with get, set abstract FilterComponent: React.ReactType with get, set abstract PaginationComponent: React.ReactType with get, set abstract PreviousComponent: React.ReactType with get, set abstract NextComponent: React.ReactType with get, set abstract LoadingComponent: React.ReactType with get, set abstract NoDataComponent: React.ReactType with get, set abstract ResizerComponent: React.ReactType with get, set type [] TextProps = /// Default: 'Previous' abstract previousText: React.ReactNode with get, set /// Default: 'Next' abstract nextText: React.ReactNode with get, set /// Default: 'Loading...' abstract loadingText: React.ReactNode with get, set /// Default: 'No rows found' abstract noDataText: U2 with get, set /// Default: 'Page' abstract pageText: React.ReactNode with get, set /// Default: 'of' abstract ofText: React.ReactNode with get, set /// Default: 'rows' abstract rowsText: string with get, set module Column = /// Basic column props type [] Basics = /// Default: undefined, use table default abstract sortable: bool option with get, set /// Default: true abstract show: bool with get, set /// Default: 100 abstract minWidth: float with get, set /// Default: undefined, use table default abstract resizable: bool option with get, set /// Default: undefined, use table default abstract filterable: bool option with get, set /// Default: ... abstract sortMethod: SortFunction option with get, set /// Default: false abstract defaultSortDesc: bool option with get, set /// Used to render aggregated cells. Defaults to a comma separated list of values. abstract Aggregated: TableCellRenderer with get, set /// Used to render a pivoted cell abstract Pivot: PivotRenderer with get, set /// Used to render the value inside of a Pivot cell abstract PivotValue: TableCellRenderer with get, set /// Used to render the expander in both Pivot and Expander cells abstract Expander: TableCellRenderer with get, set /// Configuration of a columns cell section type [] CellProps = /// Default: undefined /// A function that returns a primitive, or JSX / React Component abstract Cell: TableCellRenderer with get, set /// Set the classname of the `td` element of the column abstract className: string with get, set /// Set the style of the `td` element of the column abstract style: obj with get, set abstract getProps: ReactTableFunction with get, set /// Configuration of a columns header section type [] HeaderProps = /// Default: undefined /// A function that returns a primitive, or JSX / React Component abstract Header: TableCellRenderer with get, set /// Set the classname of the `th` element of the column abstract headerClassName: string with get, set /// Default: {} /// Set the style of the `th` element of the column abstract headerStyle: obj with get, set /// Default: (state, rowInfo, column, instance) => ({}) /// A function that returns props to decorate the `th` element of the column abstract getHeaderProps: ReactTableFunction with get, set /// Configuration of a columns footer section type [] FooterProps = /// Default: undefined /// A function that returns a primitive, or JSX / React Component abstract Footer: TableCellRenderer with get, set /// Default: string /// Set the classname of the `td` element of the column's footer abstract footerClassName: string with get, set /// Default: {} /// Set the style of the `td` element of the column's footer abstract footerStyle: obj with get, set /// Default: (state, rowInfo, column, instance) => ({}) /// A function that returns props to decorate the `th` element of the column abstract getFooterProps: ReactTableFunction with get, set /// Filtering related column props type [] FilterProps = /// Default: false abstract filterAll: bool with get, set /// A function returning a boolean that specifies the filtering logic for the column /// 'filter' == an object specifying which filter is being applied. Format: {id: [the filter column's id], value: [the value the user typed in the filter field], /// pivotId: [if filtering on a pivot column, the pivotId will be set to the pivot column's id and the `id` field will be set to the top level pivoting column]} /// 'row' || 'rows' == the row (or rows, if filterAll is set to true) of data supplied to the table /// 'column' == the column that the filter is on abstract filterMethod: U2 with get, set /// Default: false abstract hideFilter: bool with get, set /// Default: ... abstract Filter: FilterRender with get, set type [] ExpanderDefaults = /// Default: false abstract sortable: bool with get, set /// Default: false abstract resizable: bool with get, set /// Default: false abstract filterable: bool with get, set /// Default: 35 abstract width: float with get, set type [] PivotDefaults = /// Will be overriden in methods.js to display ExpanderComponent abstract render: TableCellRenderer with get, set type Column = Column type [] Column<'D> = inherit obj inherit obj inherit obj inherit obj inherit obj /// Property name as string or Accessor abstract accessor: Accessor<'D> option with get, set /// Conditional - A unique ID is required if the accessor is not a string or if you would like to override the column name used in server-side calls abstract id: string option with get, set /// No description abstract aggregate: Aggregator option with get, set /// Default: undefined - A hardcoded width for the column. This overrides both min and max width options abstract width: float option with get, set /// Default: undefined - A maximum width for this column. abstract maxWidth: float option with get, set /// Turns this column into a special column for specifying expander and pivot column options. /// If this option is true and there is NOT a pivot column, the `expanderDefaults` options will be applied on top of the column options. /// If this option is true and there IS a pivot column, the `pivotDefaults` options will be applied on top of the column options. /// Adding a column with the `expander` option set will allow you to rearrange expander and pivot column orderings in the table. /// It will also let you specify rendering of the header (and header group if this special column is placed in the `columns` option of another column) and the rendering of the expander itself. abstract expander: bool option with get, set /// Header Groups only abstract columns: Array> option with get, set /// Turns this column into a special column for specifying pivot position in your column definitions. /// The `pivotDefaults` options will be applied on top of this column's options. /// It will also let you specify rendering of the header (and header group if this special column is placed in the `columns` option of another column) abstract pivot: bool option with get, set type ColumnRenderProps = ColumnRenderProps type [] ColumnRenderProps<'D> = /// Sorted data. abstract data: ResizeArray<'D> with get, set /// The column. abstract column: Column<'D> with get, set type [] RowRenderProps = inherit obj /// Whenever the current row is expanded abstract isExpanded: bool option with get, set /// The current cell value abstract value: obj option with get, set type [] RowInfo = /// Materialized row of data abstract row: obj option with get, set /// The post-accessed values from the original row abstract rowValues: obj option with get, set /// The index of the row abstract index: float with get, set /// The index of the row relative to the current page abstract viewIndex: float with get, set /// The size of the page abstract pageSize: float with get, set /// The index of page abstract page: float with get, set /// The nesting depth (zero-indexed) abstract level: float with get, set /// The nesting path of the row abstract nestingPath: ResizeArray with get, set /// A boolean stating if the row is an aggregation row abstract aggregated: bool with get, set /// A boolean stating if the row is grouped by Pivot abstract groupedByPivot: bool with get, set /// An array of any expandable sub-rows contained in this row abstract subRows: ResizeArray with get, set /// Original object passed to row abstract original: obj option with get, set type [] CellInfo = inherit RowInfo inherit obj abstract isExpanded: bool with get, set abstract column: Column with get, set abstract value: obj option with get, set abstract pivoted: bool with get, set abstract expander: bool with get, set abstract show: bool with get, set abstract width: float with get, set abstract maxWidth: float with get, set abstract tdProps: obj option with get, set abstract columnProps: obj option with get, set abstract classes: ResizeArray with get, set abstract styles: obj with get, set type FinalState = FinalState type [] FinalState<'D> = inherit TableProps<'D> abstract frozen: bool with get, set abstract startRow: float with get, set abstract endRow: float with get, set abstract pageRows: float with get, set abstract padRows: float with get, set abstract hasColumnFooter: bool with get, set abstract hasHeaderGroups: bool with get, set abstract canPrevious: bool with get, set abstract canNext: bool with get, set abstract rowMinWidth: float with get, set abstract allVisibleColumns: Array> with get, set abstract allDecoratedColumns: Array> with get, set abstract resolvedData: ResizeArray with get, set abstract sortedData: ResizeArray with get, set abstract headerGroups: ResizeArray with get, set type [] ReactTable<'D> = inherit React.Component type [] ReactTableStatic = [] abstract Create: unit -> ReactTable<'D> type Instance = Instance type [] Instance<'D> = inherit ReactTable<'D> abstract context: obj option with get, set abstract props: obj with get, set abstract refs: obj option with get, set abstract state: FinalState<'D> with get, set abstract filterColumn: [] props: ResizeArray -> obj option abstract filterData: [] props: ResizeArray -> obj option abstract fireFetchData: [] props: ResizeArray -> obj option abstract getDataModel: [] props: ResizeArray -> obj option abstract getMinRows: [] props: ResizeArray -> obj option abstract getPropOrState: [] props: ResizeArray -> obj option abstract getResolvedState: [] props: ResizeArray -> obj option abstract getSortedData: [] props: ResizeArray -> obj option abstract getStateOrProp: [] props: ResizeArray -> obj option abstract onPageChange: PageChangeFunction with get, set abstract onPageSizeChange: PageSizeChangeFunction with get, set abstract resizeColumnEnd: [] props: ResizeArray -> obj option abstract resizeColumnMoving: [] props: ResizeArray -> obj option abstract resizeColumnStart: [] props: ResizeArray -> obj option abstract sortColumn: [] props: ResizeArray -> obj option abstract sortData: [] props: ResizeArray -> obj option ```

• The missing react dependency/namespace is easy to fix with a simple open Fable.Import so that React.xxxxx is valid.
• the module TableCellRenderer has a few functions that convert to/from U2 cases, and the names are of the form isCASENAME, ofCASENAME, asCASENAME, and for the second case (React.ReactNode), the full name of React.ReactNode is used giving ofReact.ReactNode. Do the generated names need to be normalized here?
• module PivotRenderer has a similar deficiency
• the TableProps<'D, 'ResolvedData> type inherits from some partial types in the typescript: Partial, Partial, Partial, Partial, Partial, Partial, but in the generated code each of these is represented byinherit obj, leading to repeated instances ofinherit obj` in the type definition:
  
• the noDataText property of TextProps's second case is a React.ComponentType which is unknown to me
• the ReactTable<'D> type inherits from React.Component<obj> which expects two type parameters.

I'm not really sure where to begin to investigate each of these issues, but I'd love to help triage or dig further.

[MangelMaxime]

Hello

* The missing react dependency/namespace is easy to fix with a simple `open Fable.Import` so that `React.xxxxx` is valid.

This is "normal", because ts2fable generate codes that he can understand. So if all the libraries were generated using ts2fable and if this tool was perfect then the important would work. But this is not the case, and in general we use ts2fable to do all the heavy work and then fix/improve manually the binding.

* the module `TableCellRenderer` has a few functions that convert to/from U2 cases, and the names are of the form `isCASENAME`, `ofCASENAME`, `asCASENAME`, and for the second case (`React.ReactNode`), the full name of `React.ReactNode` is used giving `ofReact.ReactNode`. Do the generated names need to be normalized here?

All these codes can be deleted because it doesn't work anymore with Fable 2. More info here

* the `TableProps<'D, 'ResolvedData>` type inherits from some partial types in the typescript:     Partial, Partial, Partial, Partial, Partial, Partial`, but in the generated code each of these is represented by `inherit obj`, leading to repeated instances of `inherit obj` in the type definition:

This is probably something not yet supported by ts2fable. You will have to go the manual way for this type.

• the noDataText property of TextProps's second case is a React.ComponentType which is unknown to me

• the ReactTable<'D> type inherits from React.Component<obj> which expects two type parameters.

We could say ts2fable is generated code that ts2fable understand. So it generated code for React as it would have generated react bindings. Source

Fable.React has been manually reworked in order to be easier to use with F# this why you don't know these types. You can take a look at the source code I linked and find/guess the equivalent in Fable.React (and people do use Fable.React and not the generated React.fs :) ).

[baronfel]

Thanks for the pointers @MangelMaxime, I'll take a look and see if I can make those modifications and more importantly understand the reasoning behind them 👍