search

novaksolutions / infusionsoft-php-sdk

An OO Infusionsoft PHP SDK
Other
69 stars 53 forks source link

README still mentions no dependencies #170

Closed Ultimater closed 3 years ago

Ultimater commented 3 years ago

The README.md mentions

No dependencies. If your server has PHP and cURL, then you are good to go!

Yet six months ago a dependency was added for handling UTF-8 characters: https://github.com/novaksolutions/infusionsoft-php-sdk/commit/7364429ec48f93f4f06607cbc70a8d544b273570#diff-d2ab9925cad7eac58e0ff4cc0d251a937ecf49e4b6bf57f8b95aab76648a9d34

joeynovak commented 3 years ago

That is a great point :)

I am usually a huge fan of no dependencies, it is still an option to pull the composer dependency in and make it so it doesn't require any, but I am not as sensitive to light-weight dependencies anymore.

What are your thoughts? Pros and cons of having a dependency from your prespective?

Ultimater commented 3 years ago

@joeynovak I'm all for dependencies. This way there's less time spent re-creating the wheel.

Although some packages can run into conflicts such as a Guzzle 6 and Guzzle 7. If you use a package that relies on one and you use another package that relies on the other, boom, conflict. Generally this sort of thing can be resolved by picking package versions where the two Guzzle versions are compatible. Does this prevent people from using Guzzle? Not at all. Look at laravel for example which updated their guzzle version with newer Laravel versions. People just create new releases of their packages when updating guzzle and the community does well.

I don't really see any cons with a dependency. People would still be doing composer install either way.

But I'd sooner let them continue maintaining the package, and just update the README.

What wording to use on the README is beyond me though.

Could just copy-paste their one file: https://github.com/neitanod/forceutf8/blob/master/src/ForceUTF8/Encoding.php

But I also like piggybacking off other's work.

So maybe just update the README.

jchimene commented 3 years ago

I solved this with Composer almost 6 months ago, and in the process found a long-standing bug in composer.json, submitted a PR => crickets. Thoughts?

Ultimater commented 3 years ago

I solved this with Composer almost 6 months ago, and in the process found a long-standing bug in composer.json, submitted a PR => crickets. Thoughts?

What would you do different?

Ultimater commented 3 years ago

@jchimene Ah, I didn't realize you were referring to a different repo.

I see your PR over there deletes a bunch of files and points to this repo for its files at dev-master. I also see it uses neitanod/forceutf8. I don't see how your PR over there "solves" this issue, if anything it adds to the problem by switching over to more dependencies.

The issue is we're introducing dependencies, and the README over here says there's no dependencies.

So what course of action should be taken? Merely update the README and remove the no dependencies mention? Or do some sort of code refactoring, getting rid of dependencies, so the README is accurate?

joeynovak commented 3 years ago

The README.md mentions

No dependencies. If your server has PHP and cURL, then you are good to go!

Yet six months ago a dependency was added for handling UTF-8 characters: 7364429#diff-d2ab9925cad7eac58e0ff4cc0d251a937ecf49e4b6bf57f8b95aab76648a9d34

Thanks for bringing this to my attention and your thoughts on dependencies. They align with my thoughts as well. I'll update the readme :)

jchimene commented 3 years ago

I'm going to delete the other PR. It's obvious you either don't understand or don't want it. This entire episode has been insulting beyond belief.