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

Add event type as constants to Event class #443

Merged
merged 1 commit into from
Feb 26, 2018
Merged

Add event type as constants to Event class #443

merged 1 commit into from
Feb 26, 2018

Conversation

nickdnk
Copy link
Contributor

@nickdnk nickdnk commented Feb 16, 2018

Hello again

I often find myself using the raw string values in my project. I wanted to avoid this, so I added them as constants to the library. This avoids typos in event type strings and also makes it easy to find the event type one is looking for without having to consult the online docs.

:)

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 16, 2018

Now i'm gonna try the rebase we just talked about @brandur-stripe :)
Hold on

Edit: When I run the rebase command it throws me into some vim-editor that I cannot figure out how to use :(

lib/Event.php Outdated
const TRANSFER_CREATED = 'transfer.created';
const TRANSFER_REVERSED = 'transfer.reversed';
const TRANSFER_UPDATED = 'transfer.updated';
const PING = 'ping';
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just keep this one alphabetized with the others.

Copy link
Contributor Author

@nickdnk nickdnk Feb 16, 2018

Choose a reason for hiding this comment

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

It follows the exact order of the events on https://stripe.com/docs/api#event_types
But I can move it up if you still want?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, move it. We should probably change it in the API reference too.

Copy link
Contributor Author

@nickdnk nickdnk Feb 16, 2018

Choose a reason for hiding this comment

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

I think the reason it's at the bottom is because it's the only one that has no object attached (all others "describe" something). In other words; it's not really an event you can process in any way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. I just think that in this case it'd be better to keep alphabetical order because that sort of information isn't really present. It's generally expected that this sort of list of constants is arranged.

@brandur-stripe
Copy link
Contributor

Nice! Thanks for the big chunk of work here.

I like the constants, but there is also a bit of a disadvantage in that it gets easier for this canonical list of events to become outdated more easily (we already have this probably elsewhere like in PHPDoc properties so maybe it's not the end of the world though).

@ob-stripe Thoughts?

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 16, 2018

It may become outdated as in "missing some events", but it can never become outdated as in "has wrong values" as this would effectively break everyone's current implementation of webhooks.

@brandur-stripe
Copy link
Contributor

brandur-stripe commented Feb 16, 2018

Edit: When I run the rebase command it throws me into some vim-editor that I cannot figure out how to use :(

https://i.imgur.com/VOe71EA.png

Try setting EDITOR temporarily to something you know how to use. For example:

export EDITOR=nano

Then try the rebase again.

It may become outdated as in "missing some events", but it can never become outdated as in "has wrong values" as this would effectively break everyone's current implementation of webhooks.

Yes, I meant that it might be missing newer events.

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 16, 2018

@brandur-stripe
I tried following your guide from the last PR, but I end up here: https://imgur.com/a/0zuML

Not sure what to do and I'm afraid to push now which may make it worse :(

@brandur-stripe
Copy link
Contributor

brandur-stripe commented Feb 16, 2018

I tried following your guide from the last PR, but I end up here: https://imgur.com/a/0zuML

This actually looks roughly like where you want to be ... as noted in that other PR, you want to change all but the top commit to f instead of pick, write out the file, then exit.

If you didn't change anything in there, you won't have seen the "rebasing ..." message afterwards and your branch should be safe. You can also just test with the standard git push origin master — Git won't let you push a rebased branch without specifying the --force flag.

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 16, 2018

I got it. They're squashed into one commit now :) Great

@brandur-stripe
Copy link
Contributor

I got it. They're squashed into one commit now :) Great

Nice work! Now just follow that process every time you add a new commit :)

I'll wait for a second opinion from OB, but otherwise this is looking good. Thanks!

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 16, 2018

I figured that most devs will likely be creating these constants on their own (at least I was), and those are just as likely (if not more) to become outdated as this list is. I was debating with myself where to add my constants but then figured I might as well complete the list and attach them to the Event class for everyone to use.

And you're welcome :) Stripe <3

@ob-stripe
Copy link
Contributor

I like the idea of having constants defined in the library! Just a suggestion: do you think it would make sense to wrap all the constants in their own class/namespace, to clarify that all those constants represent event types?

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 16, 2018 via email

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 17, 2018 via email

@ob-stripe
Copy link
Contributor

@nickdnk Do you mind opening a separate PR for the PHPDocs stuff? It's better if each PR only covers one self-contained set of changes.

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 19, 2018

@ob-stripe I would very much like to, but how do I do that when I already have a fork on my page with the previous PR? I am not sure how to split it into two.

@ob-stripe
Copy link
Contributor

You'd need to create a new branch for the second PR, then use the cherry-pick command to pick the commit into the new branch. Something like this should work:

git checkout master
git checkout -b fix-phpdoc
git cherry-pick c93ac5c

Once you've picked the commit, you can push the new branch and open another PR.

Afterwards, you can go back to the add_event_constants branch, reset to the previous commit and force-push to update the PR:

git checkout add_event_constants
git reset --hard HEAD~1
git push origin HEAD --force

Beware that this will delete the commit, so don't do this until you're sure the commit has successfully been picked in the other branch.

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 19, 2018

@ob-stripe I think I managed to do it correctly. Thanks for the assistance :)

@ob-stripe
Copy link
Contributor

Awesome :) Can I bother you to rebase this PR to squash the 2 commits into one?

Moved ping to alphabetical position
@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 19, 2018

@ob-stripe Done :)

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 26, 2018

Hey @ob-stripe - would you mind merging this one in? Or do I need to change something? :)

@ob-stripe ob-stripe self-requested a review February 26, 2018 12:34
@ob-stripe ob-stripe self-assigned this Feb 26, 2018
@ob-stripe ob-stripe merged commit f6fea77 into stripe:master Feb 26, 2018
@ob-stripe
Copy link
Contributor

Sorry for the delay @nickdnk -- just released this in stripe-php 6.3.1. Thanks for the contribution :)

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 26, 2018

Not a problem. I'm happy to help :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants