Merge changes in Rc/v2.1.0 to main branch

[mingstat]

Improved code quality through refactoring for better readability and maintainability, resolved partial matching issues for file names without extensions, and enhanced the load_data() function with new env_var and print_file_paths arguments for greater flexibility.

[ml-ebs-ext]

Whole paths would be better, because file_paths can point to different folders with files that share the same name.

[mingstat]

The use of base file names as list names is maintained to ensure backward compatibility with existing code that relies on the legacy load_data() function. For users who need the full file paths, this information is stored in the metadata attributes of each data frame in the returned list.

[ml-ebs-ext]

Users of legacy load_data() retain the old behavior because of this statement that happens at the end of that function:

names(data_list) <- file_names

Users of load_data_files() would benefit from seeing the exact path they provided as names of the output list.

[ml-ebs-ext]

See discussion in internal chat about possibility of removing both the paths and the extensions as well as preventing repeat entries in the resulting list. The

names(data_list) <- file_names

at the end of load_data() should make that function immune to implementing this change.

[ml-ebs-ext]

I don't get the rewrite from toupper to tolower, the change in the error message, the moving of code around and the obvious comments. They only open the doors for a bug to creep in and make code review a longer process for no discernible benefit.

[mingstat]

The change to lowercase file extensions (.rds and .sas7bdat) was made to align with common R package conventions, as seen in packages like {pins} and {haven}.

[ml-ebs-ext]

The old toupper is only used to do a case-insensitive check of the file extension. It's a non-user-facing implementation detail. Changing the logic is unnecessary.

[ml-ebs-ext]

There's a lot of new code here and I haven't reviewed it closely. The difference that strikes me the most is that the old toupper case-insensitive match behavior is gone. I imagine this can have an impact under Windows. Since load_data has been rewritten to list files through this function, we need a good reason to deviate from the old behavior.

[mingstat]

The current code follows the case-sensitive behavior of readRDS() and haven::read_sas() to avoid ambiguity and risk of matching the wrong file.

[ml-ebs-ext]

This piece of code belongs to the create_data_list currently on the main branch:

# Case insensitive file name match
uppercase_file_name <- toupper(paste0(x, ext))

match_count <- sum(uppercase_candidates == uppercase_file_name)
if (match_count > 1) {
  stop(paste("create_data_list(): More than one case-insensitive file name match for", file_path, x))
}

It is there to warn against an edge-case scenario in which a folder contains two files that share the same name but differ in case. That is not a problem under linux, but we still want to warn users against that situation, because running the same code with the same data files under case-insensitive windows file systems could lead to the loading of different files.

This check is no longer in the rewritten dv.loader and it should be, unless the team decides otherwise.

My suggestion here would be to take the original logic of create_data_list and adapt it minimally to follow the old filename-matching logic, so that we don't throw away useful behavior on a rewrite.

[mingstat]

Thank you for raising this important point about case-sensitivity. I agree we should discuss with the team to determine if we want to to make any changes to the current behavior.

[mattkorb]

This variable is not used anywhere. I think it can be removed.

[mingstat]

Good catch. I've removed the unused file_ext variable since it's now handled in the get_file_paths() function.

[ml-ebs-ext]

Here are my thoughts on the last changes to this PR.

Context

The "partial matching" issue can't be fixed because it's documented behavior of dv.loader v2.

The way we want to that address that issue is to break the load_data function into two:

• The one that does the partial matching, which up to this PR was called get_file_paths.
• The one that does the loading, which was and is called load_data_files.
  We can recreate the load_data function by composing it as two call to these two new functions.

Comments

On my last review of the code, I complained that an important part of the original logic of the
legacy load_data had been dropped and that we needed to recover it.

I suggested to

take the original logic of create_data_list and adapt it minimally to follow the old filename-matching
logic, so that we don't throw away useful behavior on a rewrite.

Now I see that get_file_paths is gone and create_data_list is back, which not only lists paths,
but also loads their contents through load_data_files.
For me this is one step forward and one step back. Because indeed, we recover the old file matching
logic, but now load_data calls create_data_list which in turn calls load_data_files once per file
(inside a lapply) even though load_data_files already allows to load multiple files at the same time.
I find that unnecessarily confusing and it's the first thing I'd address if I was contributing to this codebase.

Recommendation

• Revert these two commits:
• 350f4ed
• b971bb9
• Adapt get_file_paths to use the legacy matching logic in the old create_data_list.

Alternatively, we could get fresher eyes than mine on this PR. I think we're going in circles and I'm not helping.
None of my comments are set to block the approval of this PR and I'm more than willing to step aside.

[mingstat]

Here are my thoughts on the last changes to this PR.

Context

The "partial matching" issue can't be fixed because it's documented behavior of dv.loader v2.

The way we want to that address that issue is to break the load_data function into two:

• The one that does the partial matching, which up to this PR was called get_file_paths.
• The one that does the loading, which was and is called load_data_files.
  We can recreate the load_data function by composing it as two call to these two new functions.

Comments

On my last review of the code, I complained that an important part of the original logic of the legacy load_data had been dropped and that we needed to recover it.

I suggested to

take the original logic of create_data_list and adapt it minimally to follow the old filename-matching
logic, so that we don't throw away useful behavior on a rewrite.

Now I see that get_file_paths is gone and create_data_list is back, which not only lists paths, but also loads their contents through load_data_files. For me this is one step forward and one step back. Because indeed, we recover the old file matching logic, but now load_data calls create_data_list which in turn calls load_data_files once per file (inside a lapply) even though load_data_files already allows to load multiple files at the same time. I find that unnecessarily confusing and it's the first thing I'd address if I was contributing to this codebase.

Recommendation

• Revert these two commits:
• 350f4ed
• b971bb9
• Adapt get_file_paths to use the legacy matching logic in the old create_data_list.

Alternatively, we could get fresher eyes than mine on this PR. I think we're going in circles and I'm not helping. None of my comments are set to block the approval of this PR and I'm more than willing to step aside.

Thank you for your detailed and thoughtful review. I agree with your assessment and recommendations. As suggested, I've reverted the two commits and adapted get_file_paths to use the legacy matching logic from the original create_data_list function. You can find these changes in my latest commit here: 6f65e5c

This should address the concerns about maintaining the original file matching behavior while keeping the cleaner separation between path matching and file loading functionality.

[ml-ebs-ext]

I see you really haven't reverted b971bb9. That means that the load_data in this branch behaves differently than that in main. I've stopped reviewing the changes after finding this problem.

---

This task has taken so much of my time that I'm struggling to fulfill my commitments to this and to other projects. I'm starting to use my personal time to take care of my assigned workload. This is not a sustainable situation and so,

I won't be conducting any more reviews of this codebase unless I'm explicitly asked to do it during a sprint planning meeting.

Instead, I want to offer what I believe is the main change that this task requires, built on top of the last released version of this package.

This commit adds a data load function that is devoid of the "partial matching" behavior originally reported as a problem. It follows the design the team recently outlined in our internal chat. The rest of the package is preserved as it is, so no extra code review is necessary.

If the new function is ever tested, reviewed and deemed appropriate, then someone should modify the package documentation to take it into account, bump version numbers, etc. But, as stated above, that someone won't be me unless formally assigned to the task.

[mingstat]

As discussed internally, a new pull request will be created to introduce a load_files() function without making any changes to the legacy load_data() function.