Refactor export stage to use a config dict representing the workbook structure - #175
Conversation
Damonamajor
changed the title
|
|
||
| # Schema helper functions for workbook creation -------------------------------- | ||
|
|
||
| col_pos <- function(schema, col_name) { |
There was a problem hiding this comment.
I only pulled the shared helpers into this file (API and workbook) in model_res_avm. This means 2 out of the 4 small functions. But, then I noticed when doing this version that we may not even want to put them in the helper since nothing is shared (since we don't have the API workbook). Do you have any preference in how we want these 4 small functions stored between both repositories?
Just let me know if you want changes to this or the other one.
ccao-data/model-res-avm#538 (comment)
There was a problem hiding this comment.
[Suggestion, optional] I don't have strong feelings! I think it makes sense in the res model to define the functions that are specific to one script in the body of that script, and factor out shared functions in helpers.R; for the condo model, I think it makes sense to just match the organization that the res model uses, even though functions are not shared in the same way -- I find it useful for the organization of the two projects to be as close as possible, for the sake of making it easier to find things. So I would just copy the way the res model organizes these functions, even though the logic of the organization is less apparent in this context.
jeancochrane
left a comment
jeancochrane
left a comment
There was a problem hiding this comment.
Awesome work! I ran the workbook for Berwyn and double-checked it against the prod workbook -- everything matches, except for one small cosmetic bug that we fixed in this PR by switching to the new data structure (in the old code, the "Year Built" column got formatted with the sale_2_outlier conditional formatting, probably due to an off-by-one error with the column indexes).
|
|
||
| # Schema helper functions for workbook creation -------------------------------- | ||
|
|
||
| col_pos <- function(schema, col_name) { |
There was a problem hiding this comment.
[Suggestion, optional] I don't have strong feelings! I think it makes sense in the res model to define the functions that are specific to one script in the body of that script, and factor out shared functions in helpers.R; for the condo model, I think it makes sense to just match the organization that the res model uses, even though functions are not shared in the same way -- I find it useful for the organization of the two projects to be as close as possible, for the sake of making it easier to find things. So I would just copy the way the res model organizes these functions, even though the logic of the organization is less apparent in this context.
| # 5.2. Building-Level -------------------------------------------------------- | ||
|
|
||
| # Get range of rows in the building data + number of header rows | ||
| bldg_row_range <- 5:(nrow(assessment_pin10_filtered) + 6) |
There was a problem hiding this comment.
[Suggestion, optional] Similar to the code in the res model prior to ccao-data/model-res-avm#538, I think the count of header rows here (6) is wrong because it's referring to the PIN detail sheet header row count, rather than the building detail sheet. If we want the two repos to match as closely as possible, we could create another variable for the header row count for the building-level sheet, and use it here; however, I don't think it's a huge deal, because the consequence of this count being wrong are minimal (we're just adding styles to two extra rows).
| rows = bldg_row_range, cols = style_cols, gridExpand = TRUE | ||
| ) | ||
| } | ||
| addFilter(wb, bldg_sheet_name, 4, seq_along(bldg_schema)) |
There was a problem hiding this comment.
[Nitpick, optional] The 4 here is another spot that could benefit from a header row count variable.
There was a problem hiding this comment.
[Nitpick, optional] Another spot that could benefit from a header row count variable.
jeancochrane
left a comment
jeancochrane
left a comment
There was a problem hiding this comment.
Thanks for porting this logic over!
| cols = 27:31, | ||
| cols = cols_with_cond(pin_detail_schema, "sale_outlier_1"), | ||
| rows = pin_row_range, | ||
| style = createStyle(bgFill = "#FFCC99"), | ||
| rule = "$AE7=2", | ||
| rule = paste0("$", sale1_num_parcels_col, num_head + 1, "=2"), | ||
| type = "expression" | ||
| ) | ||
| conditionalFormatting( | ||
| wb, pin_sheet_name, | ||
| cols = 32:37, | ||
| cols = cols_with_cond(pin_detail_schema, "sale_outlier_2"), | ||
| rows = pin_row_range, | ||
| style = createStyle(bgFill = "#FFCC99"), | ||
| rule = "$AJ7=2", | ||
| rule = paste0("$", sale2_num_parcels_col, num_head + 1, "=2"), | ||
| type = "expression" | ||
| ) | ||
|
|
||
| # Format sale columns such that they are red if the sale has an outlier flag | ||
| conditionalFormatting( | ||
| wb, pin_sheet_name, | ||
| cols = 27:31, | ||
| cols = cols_with_cond(pin_detail_schema, "sale_outlier_1"), | ||
| rows = pin_row_range, | ||
| style = createStyle(bgFill = "#FF9999"), | ||
| rule = '$AC7!=""', | ||
| rule = paste0("$", sale1_outlier_col, num_head + 1, '!=""'), | ||
| type = "expression" | ||
| ) | ||
| # For some reason vector cols don't work with expressions, so we have | ||
| # to duplicate the conditional formatting for the sale outlier flag above | ||
| # to apply it to the second range of columns | ||
| conditionalFormatting( | ||
| wb, pin_sheet_name, | ||
| cols = 32:37, | ||
| cols = cols_with_cond(pin_detail_schema, "sale_outlier_2"), | ||
| rows = pin_row_range, | ||
| style = createStyle(bgFill = "#FF9999"), | ||
| rule = '$AH7!=""', | ||
| rule = paste0("$", sale2_outlier_col, num_head + 1, '!=""'), | ||
| type = "expression" | ||
| ) |
There was a problem hiding this comment.
[Suggestion, optional] I like how you simplified this logic using a loop in the equivalent res model PR. Not strictly necessary to port that logic over here, but I do think it would make this code a little bit easier to read, assuming there's not a reason that would prevent us from using a loop here.
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
2 participants
Companion issue to this PR
ccao-data/model-res-avm#538