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

Version 2.0 #44

Closed
wants to merge 8 commits into from
Closed

Version 2.0 #44

wants to merge 8 commits into from

Conversation

adrium
Copy link

@adrium adrium commented Jan 30, 2017

This would be an interesting version 2.0 release in my opinion 😉
Please see the readme for all changes.

*
* @internal
*/
abstract class EnumManager
Copy link

@chippyash chippyash Jan 30, 2017

Choose a reason for hiding this comment

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

Wouldn't this be better called Enum\Factory

Actually, I see more clearly what it is doing. Enum\Store would be a better name

@@ -0,0 +1,55 @@
<?php

Choose a reason for hiding this comment

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

Not seeing test for this new functionality

@adrium
Copy link
Author

adrium commented Jan 30, 2017

Thanks for reminding me to write more tests. I increased the coverage of 1.5.0 from 89% to 98%.

php-enum-coverage

Please note that EnumManager is considered implementation detail, hence the @internal tag. It is 100% covered by tests. You are welcome to rename it, if you like. Strictly speaking, I would neither consider it a factory, since it does not create any objects.

BTW: It seems that __wakeup can not be tested correctly?!

@chippyash
Copy link

chippyash commented Jan 31, 2017 via email

@adrium
Copy link
Author

adrium commented Jan 31, 2017

I like "Store" as a name. However, let's see, if there is interest to consider this as version 2 first.

@mnapoli
Copy link
Member

mnapoli commented Jan 31, 2017

Thank you for the PR and the work you put into that. However I honestly see little value in a v2 as of today:

  • it's used as a base building block so keeping BC is important
  • we want to avoid conflicts were one package would require v1, one would require v2, so you can't use both packages in the same app (like with Guzzle 5 and 6)
  • it's used a lot yet there is not a huge demand for the feature you implemented (you're the second person to ask in years)

That also doesn't answer the problem of deserialization (see #8 (comment))? (or maybe I missed it?)

@nick4fake
Copy link

@mnapoli I think ENUMs should be singletons as they are in most other languages.

Is deserialization currently the main problem? I mean, is "serialize" used widely for ENUMs?

@mnapoli
Copy link
Member

mnapoli commented Mar 12, 2017

@nick4fake given the package is installed around 50 000 times per month through Composer we can imagine it's being serialized in some projects.

At this point in the life of an open source project stability is much more important than being perfect on every aspect.

Let's put that in context, we are talking about being able to use Foo::BAR() === Foo::BAZ() instead of Foo::BAR() == Foo::BAZ().

However after thinking about it some more, maybe there's a way to release a new minor version (1.* I mean) that allows (and even encourages) to have singleton values, while keeping compatibility with existing code?

Because if we can keep compatibility with the current way of doing things + we change the documentation and provide tools to let new enums be singletons, then I'm all for it :) We could even deprecate the "old" way to encourage everyone to switch to the new way.

@adrium
Copy link
Author

adrium commented Aug 28, 2017

maybe there's a way to release a new minor version [...] while keeping compatibility with existing code?

Have a look at #43, all previous tests (and one new) still passing. Just rebased onto the new master branch.

@adrium
Copy link
Author

adrium commented Aug 28, 2017

I very much understand to keep compatibility! However, I rebased 2.x onto master just for demonstration and review.

@adrium
Copy link
Author

adrium commented Aug 29, 2017

I have more time to reply now. Thanks for the interesting discussion.
I haven't thought about packages requiring different versions... Right, composer does not support that scenario. Given that, may I fork and rename the project as adrium/php-enum2?

@mnapoli
Copy link
Member

mnapoli commented May 7, 2018

Thank you for your work but I think a v2 would be damaging for the community as it would create a lot of conflicts downstream. If a v2 has enough value this would offset the pain, but at the moment I do not think it's worth it.

@mnapoli mnapoli closed this May 7, 2018
@dbalabka
Copy link

@mnapoli, I ended up with a different implementation of enums that address multiple issues in the backlog. Maybe it might become the next major version of php-enum. It addresses the issue with singletons that are referring @nick4fake and describe the workaround for serialization issue.

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.

5 participants