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

Added to Resend.php Facade: Contact, Audience and Batch methods #70

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

Conversation

cheddZy
Copy link

@cheddZy cheddZy commented Nov 30, 2024

Small addition for missing methods to the Facade for better developer UX.

@jayanratna
Copy link
Collaborator

Thanks for submitting this PR @cheddZy

This is a great addition but I think we can make it better. Instead of using actual methods in the Facade class, we should add a comment to the top of the class to make the developer experience better.

Something like the Laravel DB Facade:

<?php

namespace ...;

/**
 * @method static \Illuminate\Database\ConnectionInterface connection(string $name = null)
 * @method static \Illuminate\Database\Query\Builder table(string $table, string $as = null)
 ...
 * @see \Illuminate\Database\DatabaseManager
 */

The reason I suggest this is because the underlying class that supports this Facade already has the methods that the Facade class defines. Eg. Resend::contacts()

@cheddZy
Copy link
Author

cheddZy commented Nov 30, 2024

Hey - great suggestion, I'll get right on that. Completely slipped my mind we can do it with PHPDoc this way!

@cheddZy
Copy link
Author

cheddZy commented Nov 30, 2024

Preoccupied right now but I noticed that I forgot to add some PHPDoc comments for the methods inside the class. I'll get on that in a little bit.

@cheddZy
Copy link
Author

cheddZy commented Dec 1, 2024

Would like to mention that I believe the class originally only has properties and not the methods. Could I get a sanity check on this from someone?
image

@jayanratna
Copy link
Collaborator

I've approved automated tests for this PR. I'll also do some manual testing shortly.

@jayanratna
Copy link
Collaborator

Apologies that I didn’t get time to review this earlier, but I’m really liking the latest changes. 💯

@cheddZy
Copy link
Author

cheddZy commented Dec 19, 2024

No worries, thanks for the quick reply! Whenever you get the time for it let me know if there's anything else I can improve. It's a pretty small change but hoping it's meaningful enough.

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