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

Embrace extendability and make all private Transport methods protected #33

Merged
merged 1 commit into from Sep 5, 2020
Merged

Conversation

mfn
Copy link
Contributor

@mfn mfn commented Jul 9, 2020

Summary

While trying to fix #32 locally
by overriding the send() method, I realized I couldn't do this because send()
calls private methods I can't access.

Since the class already contains a mix of protected/private/public, I suggest to
make everything "protected by default".

For a read on the philosophy on this, I would like to suggest to read
http://fabien.potencier.org/pragmatism-over-theory-protected-vs-private.html
for inspiration.

While trying to fix #32 locally
by overriding the `send()` method, I realized I couldn't do this because `send()`
calls private methods I can't access.

Since the class already contains a mix of protected/private/public, I suggest to
make everything "protected by default".

For a read on the philosophy on this, I would like to suggest to read
http://fabien.potencier.org/pragmatism-over-theory-protected-vs-private.html
for inspiration.
Copy link
Contributor

@vladsandu vladsandu left a comment

Choose a reason for hiding this comment

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

@mfn thank you for sharing that. It was a good read.

I'm generally a bit torn in regards to this philosophy, but I agree that for this particular library, it would be a good fit and the added extensibility would be valuable.

I will be including this in the next release, which I'll be doing later today.

@vladsandu vladsandu merged commit b6dd7ba into ActiveCampaign:master Sep 5, 2020
@mfn mfn deleted the mfn-protected branch September 5, 2020 12:05
@mfn
Copy link
Contributor Author

mfn commented Sep 5, 2020

I'm generally a bit torn in regards to this philosophy

I can understand and if I may be honest, it took me some time to digest and formalize my mind regarding this.

In any non-OSS codebase I worked on, there's basically only public or private; rarely protected (only when actual inheritance is relevant).

But what I've seen over the last half decade of contribution in the OSS world is this:

  • more people were helped with in that that they had the flexibility to override protected methods
    vs
  • the minority of people which were hit by backwards incompatible changes because non-documented internal methods did change unexpectedly, even in minor versions

Ultimately making it protected kind of makes part of the contract of users of the library and does increase the complexity of handling compatibility / releases / adherence to semver.


On a higher philosophical level, the "Open" in "Open Source Software" does in way for me reflect this "openness" also in actual code. I don't think I'll ever write private in library code again.

Thanks for merging!

@vladsandu
Copy link
Contributor

Thank you for sharing your experience on this! Those are definitely some valid points that you're making.

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.

None yet

2 participants