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

Event type #1053

Merged
merged 3 commits into from
Nov 2, 2020
Merged

Event type #1053

merged 3 commits into from
Nov 2, 2020

Conversation

casey
Copy link
Contributor

@casey casey commented Nov 1, 2020

This adds a method to get the EventType of an Event. I needed this for logging and statistics. Hopefully it's useful to others!

For some reason I was getting compilation failures, but adding the lifetime fixed them.

I also added EventType::name, which returns the screaming-snake-case name as originally returned by the API. This doesn't apply cleanly on its own, so I included it in this PR, since it's related.

@casey
Copy link
Contributor Author

casey commented Nov 1, 2020

The error on beta and nightly is very similar to the compilation error I was getting. Has that been seen before?

@arqunis
Copy link
Member

arqunis commented Nov 1, 2020

No. The lifetime you added is also wrong. The prefix function is meant to return a Cow that references the message's content (stored inside Stream). Now the Cow also references Context, which it doesn't have to. Could you remove the lifetime?

@arqunis arqunis added enhancement An improvement to Serenity. model Related to the `model` module. labels Nov 1, 2020
@casey
Copy link
Contributor Author

casey commented Nov 1, 2020

Will do, I'll push a diff in a second.

One question, would you prefer that event names, since they appear in multiple places, be factored into constants? I.e. GUILD_ROLE_DELETE instead of "GUILD_ROLE_DELETE".

@arqunis
Copy link
Member

arqunis commented Nov 1, 2020

Sure.

Add `Event::event_type` method that returns the type of the event.
This returns this original event name, in screaming snake case, as
originally returned by the Discord API.

`GuildUnavailable` is a synthetic event type, corresponding to either
`GUILD_CREATE` or `GUILD_DELETE`, but we don't have enough information
to recover the name here, so this function returns `None` if called on
`GuildUnavailable`.
@casey
Copy link
Contributor Author

casey commented Nov 2, 2020

Done, I removed the lifetime and refactored event name strings into constants. I'm not getting the compilation error anymore, so it must have been something wrong on my end.

src/model/event.rs Show resolved Hide resolved
@casey
Copy link
Contributor Author

casey commented Nov 2, 2020

I believe that currently, it is only possible to omit the static lifetime for normal constants, not associated constants. (See this issue.)

As an alternative, I could make them not associate constants, i.e. move them out of impl EventType and into the containing module.

@arqunis
Copy link
Member

arqunis commented Nov 2, 2020

That's a pity. I thought that works for associated constansts as well, but it's apparently intentional that it doesn't. You can ignore my change, your pull request is fine as is. I'll merge it later. And once I also investigate the error, it is happening in CI again, this time just for the framework example...

@arqunis
Copy link
Member

arqunis commented Nov 2, 2020

I think we may have a case of a heisenbug tormenting us. I've tried testing from my copy of serenity, your copy of serenity, switching between the stable, beta, and nightly toolchains, yet I am unable to consistently reproduce the error. I also just tried restarting all of the CI jobs and the error has disappeared.

Well, I guess I have the liberty to merge your pull request, but the error is strange nonetheless.

@arqunis arqunis merged commit f3e4a6b into serenity-rs:current Nov 2, 2020
@casey casey deleted the event-type branch November 2, 2020 23:01
@casey
Copy link
Contributor Author

casey commented Nov 2, 2020

Very weird! I know I saw it locally a few times, so it must not be CI-specific.

Thank you for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement to Serenity. model Related to the `model` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants