Skip to content

Conversation

@y-bonfire
Copy link
Owner

No description provided.

@y-bonfire
Copy link
Owner Author

#11

I took some time to investigate a few things while testing the library.
Here are some casual observations that might be of interest:

Google Drive itself allows multiple files with the same title to coexist in a single folder, but this library’s file_by_title method returns only a single file.
While this design decision may have been reasonable at the time, I wonder if it's now a bit limiting—though it's probably too late to change without breaking existing usage.

While reviewing the download_to_file method, I was initially puzzled by the use of id, which isn’t explicitly defined within the class.
It turns out this is delegated via a clever but somewhat tricky mechanism in the Util module:

def download_to_file(path, params = {})
  @session.drive_service.get_file(
    id,
    **{ download_dest: path, supports_all_drives: true }.merge(params)
  )
end

The delegate_api_methods method dynamically defines methods from the api_obj on the target object, unless they’re in the exclusion list:

def delegate_api_methods(obj, api_obj, exceptions = [])
  sc = get_singleton_class(obj)
  names = api_obj.public_methods(false) - exceptions
  names.each do |name|
    next if name.to_s =~ /=$/
    sc.__send__(:define_method, name) do
      api_obj.__send__(name)
    end
  end
end

This allows access to id and other API fields, but can make things slightly harder to trace or debug, especially for new contributors.

As a minor note, the fact that a GoogleDrive::File instance is responsible for downloading itself feels slightly non-intuitive to me.
Personally, it might feel more natural if this responsibility lived in the GoogleDrive::Session, given that it's managing the connection.

Overall, the Spreadsheet side of the code felt clean, but I sensed a bit more complexity and tension in the design of the GoogleDrive side.
Perhaps that’s part of why certain issues (like the copy_to bug) weren’t addressed earlier.

Hope these reflections are helpful in some way.

@y-bonfire y-bonfire mentioned this pull request Jul 30, 2025
@y-bonfire y-bonfire merged commit 9161415 into master Jul 30, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants