Skip to content

BYOC_ID: 46896 - Refactor gpprac lookup#1335

Open
OluwatobiOni wants to merge 2 commits into
developmentfrom
refactor_gpprac_lookup
Open

BYOC_ID: 46896 - Refactor gpprac lookup#1335
OluwatobiOni wants to merge 2 commits into
developmentfrom
refactor_gpprac_lookup

Conversation

@OluwatobiOni

Copy link
Copy Markdown
Collaborator

TO-DOs:

  • Confirm and update table name in get_gpprac_ref_data() function when table is available in denodo
  • Confirm and update table name in get_gpprac_opendata() function when table is available in denodo

@OluwatobiOni OluwatobiOni requested review from Jennit07 and lizihao-anu and removed request for lizihao-anu June 5, 2026 15:26
@OluwatobiOni OluwatobiOni added On hold Waiting for something / someone outside of our control BYOC PR ready for review Preprod data not available labels Jun 5, 2026
@Jennit07 Jennit07 changed the title refactor gpprac lookup BYOC_ID: 46896 - Refactor gpprac lookup Jun 16, 2026
Comment thread R/get_gpprac_opendata.R
Comment on lines 43 to 45
postcode = phsmethods::format_postcode(.data$postcode)
) %>%
dplyr::distinct(.data$gpprac, .keep_all = TRUE) %>%

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need these lines after line 15(denodo connection)?

Comment thread R/get_lookup_paths.R
Comment on lines +186 to +192
dplyr::select(
gpprac = "praccode",
pc7 = "postcode"
) %>%
dplyr::mutate(
pc7 = phsmethods::format_postcode(.data$pc7, format = "pc7")
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we also need this above after reading the denodo connection/table?

Comment thread R/process_lookup_gpprac.R
Comment on lines +25 to +27
gpprac_ref_file <- get_gpprac_ref_data(BYOC_MODE = BYOC_MODE)
open_data <- get_gpprac_opendata(BYOC_MODE = BYOC_MODE)
spd_file <- get_spd_data(BYOC_MODE = BYOC_MODE) %>%

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we put all 3 of these into the parameters of the function please?

Comment thread R/process_lookup_gpprac.R
Comment on lines +25 to +27
gpprac_ref_file <- get_gpprac_ref_data(BYOC_MODE = BYOC_MODE)
open_data <- get_gpprac_opendata(BYOC_MODE = BYOC_MODE)
spd_file <- get_spd_data(BYOC_MODE = BYOC_MODE) %>%

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
gpprac_ref_file <- get_gpprac_ref_data(BYOC_MODE = BYOC_MODE)
open_data <- get_gpprac_opendata(BYOC_MODE = BYOC_MODE)
spd_file <- get_spd_data(BYOC_MODE = BYOC_MODE) %>%
gpprac_ref_file <- gpprac_ref_file
gpprac_open_data <- gpprac_open_data
spd_file <- spd_data %>%

This is what the code would look like if those files were added into the functions parameter above

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggested one rename here to make it a bit more clear what open data we are using

@Jennit07 Jennit07 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @OluwatobiOni Overall the PR looks good, thanks for your hard work! I have a few minor suggestions. I'm happy to have another look/test again when the data is available to us! Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants