-
Notifications
You must be signed in to change notification settings - Fork 853
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
Use \Stripe\<class>::class constant instead of strings #643
Conversation
Visually we could go with:
Like all other constant lists, if you prefer. Not that it matters, but if we're changing this anyway. EDIT: I did this. Looks cleaner as brandur said. |
Looks way cleaner to me — thanks for the follow up @nickdnk! I'll leave merge decision up to Olivier. ptal @ob-stripe |
Nice! Yeah, this seems like a strict improvement. @nickdnk There are a few other places where we use strings for class names, mind updating these too? If not I can take as a followup. |
@ob-stripe Could you point me in the direction of those? I can't really find anything of this kind elsewhere. |
I just alphabetised the list based on the constant names instead of the |
@nickdnk In the library itself, I think the only other place where we use strings for class names is in the There are a lot more class name strings in tests though, mostly in |
I updated the constants I could find in tests, but I cannot find |
@ob-stripe |
@nickdnk 🤦♂ Sorry, I forgot I removed it for the next major version. Don't mind me. Thanks for updating everything else! |
@ob-stripe No problem. Let me know when you're satisfied and I'll squash to one commit and reword it. |
… constant available in 5.6.
Squashed to single commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Nick!
I figured since we've dropped 5.5 in v7 we might as well do this. Unless of course there's a reason not to that I'm not aware of.