Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(writer): ranged writes #66

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mharrisb1
Copy link
Contributor

Closes #58

Adds ranged writes to COPY TO. This should round out the full feature release of supporting ranges.

@archiewood
Copy link
Member

archiewood commented Feb 10, 2025

What is the expected behaviour if:

  • You just pass a single cell: COPY table_name TO 'https://docs.google.com/spreadsheets/d/11QdEasMWbETbFVxry-SsD8jVcdYIT1zBQszcF84MdE8/edit?gid=1385451074#gid=1385451074&range=C6' (FORMAT gsheet)
    • I think ideally this should work. Top left of table starts C6
  • You just pass a range that is smaller than the table: COPY table_name TO 'https://docs.google.com/spreadsheets/d/11QdEasMWbETbFVxry-SsD8jVcdYIT1zBQszcF84MdE8/edit?gid=1385451074#gid=1385451074&range=C6:C7' (FORMAT gsheet).
    • Should we: allow it, starting at C6
    • block it and report error as size is too small?

@mharrisb1
Copy link
Contributor Author

Personally, I would allow it. Would actually be nice UX because you won't always know the size of the data you're writing so just clicking on a single column to get the range URL would be expected behavior

@mharrisb1
Copy link
Contributor Author

mharrisb1 commented Feb 10, 2025

It also seems like that's the behavior from the Google Sheets API since this currently works that way and I guess one of the aims is to maintain parity between extension behavior and API behavior

@Alex-Monahan
Copy link
Contributor

Personally, I would allow it. Would actually be nice UX because you won't always know the size of the data you're writing so just clicking on a single column to get the range URL would be expected behavior

One of the things I plan to use this feature for (thank you by the way!) is exactly this. I would love to just update a few columns of a spreadsheet and not have to specify the rows.

@Alex-Monahan
Copy link
Contributor

Alex-Monahan commented Feb 11, 2025

Would you be open to adding a test that uses a few whole columns as a range? Something like B:D ? Not required for sure! And if you have already tested locally, that would already be great to hear.

@Alex-Monahan
Copy link
Contributor

Another question! Does this continue the prior behavior of clearing out the whole sheet upon write? Ideally, I would love to see just the specified range get cleared out prior to insertion. If it clears out the whole sheet, I don't think it will be as easy to use for my use case. Similarly, if it is append only, I don't think it would work for what I am hoping to use it for.
Thank you!

@mharrisb1
Copy link
Contributor Author

Sure thing on adding the test. I think that's a good idea. I haven't tested that behavior myself.

As for wiping the sheet before write and having append only writes, that behavior is the same since I didn't touch any of that. Let's create a new issue to track that. I've seen some comments sprinkled throughout that make me think this isn't how we want to do it long term but I also don't want to be the one to touch that right now without some direction since I'm still getting up to speed with the codebase and why we do things the way we do.

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.

Add ranged writes to COPY TO
3 participants