Skip to content

Conversation

@owenjonesuob
Copy link
Contributor

Fixes #2213.

I had to move the "smart default" logic for .names up one level, from get_src_tbl_names() into dm_from_con() - but I think that is a better place to keep it anyway, since that's where its behaviour is documented.

I've also made sure names_pattern is used consistently as the "internal" name for this argument, wherever we see this "naming pattern" concept.

@aviator-app
Copy link
Contributor

aviator-app bot commented May 3, 2024

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

@owenjonesuob owenjonesuob force-pushed the b-pass-names-pattern-to-learn branch 2 times, most recently from 14afa3c to 192226a Compare May 3, 2024 21:40
@krlmlr krlmlr force-pushed the b-pass-names-pattern-to-learn branch from 192226a to a1119ed Compare August 14, 2024 21:13
@krlmlr krlmlr requested a review from Copilot August 1, 2025 04:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where the naming pattern was not being passed through to dm_learn_from_db(). The fix involves refactoring the "smart default" logic for naming patterns from get_src_tbl_names() to dm_from_con() and ensuring consistent use of names_pattern as the parameter name throughout the codebase.

Key changes:

  • Moved smart default logic for naming patterns from get_src_tbl_names() to dm_from_con()
  • Standardized parameter name from names to names_pattern across functions
  • Updated dm_learn_from_db() to use the correct column names with dot prefixes for glue templating

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
R/learn.R Updated dm_learn_from_db() parameter name and column selection to use dot-prefixed names
R/dm_from_con.R Added smart default logic for naming patterns and updated function calls to use names_pattern
R/db-helpers.R Removed smart default logic and updated parameter name to names_pattern
tests/testthat/test-db-helpers.R Updated test calls to use names_pattern parameter instead of names

#' }
#' @autoglobal
dm_learn_from_db <- function(dest, dbname = NA, schema = NULL, name_format = "{table}") {
dm_learn_from_db <- function(dest, dbname = NA, schema = NULL, names_pattern = "{.table}") {
Copy link

Copilot AI Aug 1, 2025

Choose a reason for hiding this comment

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

The function documentation needs to be updated to reflect the parameter name change from name_format to names_pattern. The @param documentation likely still references the old parameter name.

Copilot uses AI. Check for mistakes.
@krlmlr
Copy link
Collaborator

krlmlr commented Aug 1, 2025

@claude: Fix tests.

@krlmlr
Copy link
Collaborator

krlmlr commented Aug 1, 2025

@copilot: Fix tests.

@krlmlr
Copy link
Collaborator

krlmlr commented Aug 1, 2025

@owenjonesuob: Thank you for your patience.

@claude
Copy link
Contributor

claude bot commented Aug 1, 2025

Claude encountered an error —— View job

Failed with exit code 128

I'll analyze this and get back to you.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Naming pattern not used in dm_from_con() when keys are learned successfully

2 participants