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

Cleanup #309

Merged
merged 1 commit into from
May 19, 2023
Merged

Cleanup #309

merged 1 commit into from
May 19, 2023

Conversation

nicolas-grekas
Copy link
Contributor

I was wondering if I could replace the dependency on Guzzle by a dependency on PSR-18, but it looks like too much work for me.

Still, here are some cleanups I found while having a look:

  • update phpstan
  • update and run php-cs-fixer
  • remove GuzzleHttp\Client from the signature of methods, to make it easier to move away from it in the future if needed

It'd be great to decouple the SDK from Guzzle! 🙏

@@ -91,11 +91,6 @@ public static function getPagedResources(): string
],
];

if (empty($entities)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is dead code: $entities cannot be empty

@miguelrs
Copy link
Contributor

Hey @nicolas-grekas, thanks for taking your time away from Symfony to create this PR! ❤️ We've all been out on a whole-company trip in Mexico for the last week, but now that we're back, I'll take a look at this PR. Also, we'll try to find some time in the team in the upcoming sprints to investigate your suggestion about Guzzle 👍

@nicolas-grekas
Copy link
Contributor Author

My pleasure to help :)
Let me know what you decide for PSR18!
I'd be happy to help knowing it could be accepted.
Did Helpscout ever consider sponsoring Symfony BTW? 😉

@miguelrs
Copy link
Contributor

@nicolas-grekas I checked with the team, and we think that moving to PSR18 is generally a good idea. However, the main concern is not making the instantiation/configuration of the library more difficult for our customers (who may have limited PHP knowledge).

In fact, we used to use https://github.com/php-http/httplug to allow multiple HTTP Clients in earlier versions, but then we removed it in favor of just Guzzle for simplicity. This was several years ago though, so things may be different now!

The thing is this work is not considered a priority (and it's no small task), so with our backlog, it will likely need to wait a bit. I will try to make some room for initial discovery on this in our "special" sprint at the end of the quarter. Of course, any help would be more than welcome! 😄


As for the sponsorship, the first thing I can suggest is to check HS For Good, where I think you can get a 10% discount as a non-profit organization 💸 If you apply there, our team will look at your case.

Also, we have a Partners program, so I brought this to the attention of our partnerships team for a possible future partnership venture. If you give me a suitable contact email for this, I can pass it on to them!

And you can always reach out to help@helpscout.com to get more info -- if you have never contacted them, our customer team is the best!!

Copy link
Contributor

@miguelrs miguelrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks! I just added a couple of small suggestions if it sounds good!

phpstan.neon Outdated Show resolved Hide resolved
src/Http/Authenticator.php Outdated Show resolved Hide resolved
@miguelrs
Copy link
Contributor

@nicolas-grekas BTW, I talked to our partnerships team and explained what Symfony is, and they would like to chat with you to discuss a bit further. Unless you tell me otherwise, I will pass your @symfony.com email on to my friend Pallavi, and she will get in touch, ok?

@nicolas-grekas
Copy link
Contributor Author

Review comments addressed.
Sure, please share my email with Pallavi: nicolas.grekas before the domain name.

@miguelrs miguelrs merged commit 479a72e into helpscout:master May 19, 2023
@nicolas-grekas nicolas-grekas deleted the cleanup branch May 22, 2023 15:41
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