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 documentation for event filters #379

Merged
11 commits merged into from
Jan 10, 2016
Merged

Add documentation for event filters #379

11 commits merged into from
Jan 10, 2016

Conversation

Deamon5550
Copy link
Contributor

This adds documentation for SpongeAPI#927 Event Filters

Please feel free to edit this branch for content/formatting/whatever without asking me for permission.

@@ -0,0 +1,145 @@
======================================
Advansed Event Handling: Event Filters
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should probably change this to just 'Event Filters'

Also I spelled 'advanced' wrong...classic

- Stub for Causes page
- consistent indentation (tabs -> spaces in filters.rst, 4 spaces per
  level in custom.rst)
@ghost
Copy link

ghost commented Nov 27, 2015

Open questions of lesser priority:

@ghost ghost added API not ready The API/code is not yet ready so we cannot merge this in yet needs review The submission is ready and needs to be reviewed labels Nov 27, 2015
@Deamon5550
Copy link
Contributor Author

@Saladoc

No they cannot, I toyed briefly with how to do it but there isn't really a way for a) them to cleanly add the filters to their handler since they are implementing a method that they can't add parameters to, and b) us to cleanly generate/transform their handlers to do the filtering. So bottom line, if you're implementing EventListener yourself you're on your own for filtering.

  • @Listener(ignoreCancelled=false) vs @Listener @IsCancelled(CancellationState.IGNORE) - which one should be preferred?

@Listener(ignoreCancelled=false) is going to be removed soon and @IsCancelled will be the only place for controlling behavior around cancellation. I'll update this once I make that change.

- Listen for events
- Fire events

Overview
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say we should get rid of the Overview heading and the list above. Just some introductory sentences should suffice. If we do this we might remove it on the other topics too.

@Tzky
Copy link
Contributor

Tzky commented Nov 27, 2015

Is the concept of our events explained anywhere (Targets+Causes)? Same for the naming scheme of the events.

@ghost
Copy link

ghost commented Nov 27, 2015

The events page we have mostly deals with listeners. Causes I introduced as a stub page, issue is #267.

@Deamon5550
Copy link
Contributor Author

I'll write a page for causes sometime today

@ghost
Copy link

ghost commented Nov 28, 2015

Looks good so far. Maybe mention that NamedCause also holds constants holding common cause names so you don't have to toss around magic values.

@Deamon5550
Copy link
Contributor Author

@Saladoc I added a note to the bottom of the named cause section https://github.com/SpongePowered/SpongeDocs/pull/379/files#diff-dba86822fc60edfa471b8efba4f5b7e2R85

Events are great for the use case of wanting to execute something when certain actions occur, but
they have the drawback of providing next to no context as to what has **caused** that event to
occur. A ``Cause`` allows providing and receiving additional contextual information about the event
which can be do to modify the behavior of your event handler depending on this context.
Copy link
Member

Choose a reason for hiding this comment

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

Something is wrong with this sentence, "which can be do to modify". We can clean up some wording later, but I'm really not sure what you meant to say here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I meant to say ...which can be used to modify...

}

.. note::
Both ``@Has`` and ``@Supports`` have an optional parameter ``inverse()`` which can be set to cause validation
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe remove the brackets?

@boformer
Copy link
Contributor

Looks good.

@ghost ghost removed the API not ready The API/code is not yet ready so we cannot merge this in yet label Jan 10, 2016
@ghost ghost self-assigned this Jan 10, 2016
@ghost ghost merged commit 51dff61 into master Jan 10, 2016
@ghost ghost removed the needs review The submission is ready and needs to be reviewed label Jan 10, 2016
@ghost ghost deleted the feature/event-filters branch January 11, 2016 16:06
This pull request was closed.
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.

None yet

8 participants