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

gmail: update documentation #917

Merged
merged 7 commits into from
Jan 18, 2025
Merged

gmail: update documentation #917

merged 7 commits into from
Jan 18, 2025

Conversation

josephjclark
Copy link
Collaborator

Summary

Hi @decarteret - I was looking at the gmail adaptor on docs.openfn.org and I noticed a few issues.

I should have caught some of this stuff in review but it's not too late to fix it!

Would you mind taking a look over this before I release? It could be great if you could run a quick test too - there shouldn't be any behavioural changes but I may have missed something subtle.

Additional Changes?

I have a couple more suggestions but they're slightly bigger changes. I'm happy to implement them, although I can't so easily test (I suppose I could set up a test job eh?)

  • options.desiredContents should just be contents I think? The word "desired" doesn't add a lot, yet makes everything more verbose. To prevent breaking changes we could re-document the option as contents but alias it internally to desiredContents, so that desiredContents secretly works but contents is what users use going forward.

  • Can I also recommend that we ALWAYS include from, date and subject in the returned contents? It's hard to imagine that users wouldn't want these fields, plus they're unlikely to be particularly large in the download. Maybe we can rethink this later if any users have a strong use-case to exclude the subject. But I think in 99% of cases this will make the adaptor significantly easier to use. It might be too big of a change for now 🤔

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

packages/gmail/src/Adaptor.js Show resolved Hide resolved
packages/gmail/src/Adaptor.js Show resolved Hide resolved
packages/gmail/src/Adaptor.js Outdated Show resolved Hide resolved
packages/gmail/src/Adaptor.js Show resolved Hide resolved
Jason DeCarteret added 4 commits January 16, 2025 17:40
Modified function interface. email is typically redundant and therefore is now optional.
Refactored nested function to improve efficiency.
Added support for sticky content items alongside custom content items.
Enhanced naming convention for better code clarity.
@josephjclark
Copy link
Collaborator Author

Hey @decarteret one last thing while we're here!

Your commit introduces a breaking change. That's probably OK as you're the only user and the adaptor isn't even 48 hours old yet.

I like defaulting email to the currently authorised user, that's neat.

The only thing that strikes me - and I was thinking this earlier in the week but decided not to push it - is that query is surely a required field? Everything else is optional but you basically HAVE to provide a query. No-one wants to download the whole inbox every time. And you can just do * or something right if you really really do want to do that?

So how about I refactor the signature to:

getContentsFromMessages(query, options)

We'll sneak it in as a patch and I'll mention it in the release notes since the adaptor is so fresh.

@decarteret
Copy link
Collaborator

Hey @decarteret one last thing while we're here!

Your commit introduces a breaking change. That's probably OK as you're the only user and the adaptor isn't even 48 hours old yet.

I like defaulting email to the currently authorised user, that's neat.

The only thing that strikes me - and I was thinking this earlier in the week but decided not to push it - is that query is surely a required field? Everything else is optional but you basically HAVE to provide a query. No-one wants to download the whole inbox every time. And you can just do * or something right if you really really do want to do that?

So how about I refactor the signature to:

getContentsFromMessages(query, options)

We'll sneak it in as a patch and I'll mention it in the release notes since the adaptor is so fresh.

I recognized this and even made a note about it in the README! I think having no query is a valid use case considering "how can I pull ALL messages when query is a required field?" Sure, with "*" or "newer_than:100y" or with something crazy, anyone could figure out how to shoot themselves in the foot.

However, I think the real concern is the number of results returned. My "all messages" argument works fine when it's fresh, or utility-driven account, but it certainly breaks down when you consider a well-seasoned account will likely have tens of thousands of messages.

I'm considering adding a new parameter like resultLimit or maxResults. The default value could be 20 or 100. Could even enforce a maximum value of 1000 or something to keep people safe with some guardrails. This new limit, used in conjunction with the ability to ignore options.processedIds, would still allow the capability to process 100,000s of messages, but with more reasonable chunks.

Your thoughts?

@josephjclark
Copy link
Collaborator Author

Ok cool - query is definitely optional, I hear you.

I will merge this now with the signature as getContentsFromMessages(options)

And I will raise an issue to add a limit and offset option - or some equivalent - so that users can easily limit and paginate requests. You can pick that up if you want to, or it blocks you, or we'll leave it for someone else to pick up later.

Thanks @decarteret!

@josephjclark josephjclark merged commit 71baf16 into main Jan 18, 2025
2 checks passed
@josephjclark josephjclark deleted the gmail-docs-fixes branch January 18, 2025 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants