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

Update README.md to explain how to compare enums (singleton limitation) #148

Closed
wants to merge 1 commit into from
Closed

Conversation

alexkuc
Copy link

@alexkuc alexkuc commented Jul 14, 2021

As per my earlier comment #8 (comment), I think it is worthwhile to mention in README.md that php-enum are not singletons so when comparing them, strict comparison will not work as expected. @mnapoli Here is the PR as promised. Let me know what you think.

Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

I've added some inline comments. The gist of it is that I think we can do simpler and shorter, because this is a minor detail. It shouldn't take over the README too much.

README.md Outdated
@@ -65,6 +65,38 @@ function setAction(Action $action) {
}
```

## Comparison
Copy link
Member

Choose a reason for hiding this comment

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

It should come after the main documentation: it's a detail, it shouldn't distract from the existing documentation.

README.md Outdated
if ($action === Action::VIEW()) {
// ...
}
```
Copy link
Member

Choose a reason for hiding this comment

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

In my experience many people don't read paragraphs and copy/paste the first code block they find.

I'd rather start this section with "how to compare enums" (i.e. the code examples you have written below), and then explain that comparing enum instances with === doesn't work. No need to write more than a sentence or two TBH. No need to link to the issue either, nor use the word "workaround": this isn't really a bug, this is a minor detail that hasn't prevented this library to become the most widespread enum library in PHP.

@alexkuc
Copy link
Author

alexkuc commented Nov 17, 2021

@mnapoli I have updated the PR, let me know what you think

@mnapoli
Copy link
Member

mnapoli commented Dec 13, 2021

Hi, I'm sorry but since July PHP 8.1 now have enums. I will be closing pull requests as we all migrate to the PHP 8.1 enums.

@mnapoli mnapoli closed this Dec 13, 2021
@alexkuc
Copy link
Author

alexkuc commented Dec 18, 2021

@mnapoli Do you mean you will deprecate your package? Or you will upgrade this one to use native enums?

@mnapoli mnapoli reopened this Dec 19, 2021
@mnapoli mnapoli closed this Dec 19, 2021
@mnapoli
Copy link
Member

mnapoli commented Dec 19, 2021

(Sorry for the accidental reopen)

This package is becoming obsolete with PHP 8.1.

It will eventually be deprecated. There isn't anything this package will bring on top of PHP 8.1 enums.

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