PipePutEvent now supports additional, custom predicates

[BlvckBytes]

I would love to execute additional, custom and rather nuanced filters, which are stored on the [Pipe]-sign's PersistentDataContainer and are later parsed into an AST. All that's missing to get my desired functionality implemented is to reject items without removing them from the pipe-walk algorithm. I do not want to drop them, I simply want them to move on, as with native filters.

This rather simple and concise commit allows just that: executing a custom predicate while keeping rejected items in the system, without any measurable overhead. It would mean a lot to me if my change was to be merged, so that I can provide my feature to a small server who would most definitely decline it if they had to build from source.

[me4502]

as-is, i'm not sure what this PR is actually achieving. you're basically adding your predicate functionality into the event when it's not used by the plugin itself at all, but you're using the data in the exact same way as items that are kept as leftovers from what I can tell.

if there's some issue preventing what you're trying to do, it makes more sense to find the cause of the issue and fix it, rather than trying to add some niche functionality into the event that's not ever going to be used by either the plugin or realistically any user but yourself

[BlvckBytes]

I don't think that it's an issue which prevents me from trying to achieve my goal, but simply API-design that didn't consider my case. Items which haven't been removed from the event are tried to be added to the container, with leftovers (items that couldn't be held) being added into newItems. The entirety of filteredItems, which does not consider modifications by the event at all, are removed from the current pipe-content called items, and the leftovers from adding to the container, newItems are added back. This way, I cannot depict the case of items having been rejected from the container, because there's an additional, external filter in place, which are still supposed to continue travelling down the pipe.

I do agree that it's a sort of niche feature, but I do not understand why it would hurt the plugin in any way to have proper API in place, which can be used by other tinkerers like me. Yes, this event was meant for internal communication only, but why not make Pipes more versatile for those who wish to do so?

I'm not circumventing an issue, I'm just adding a feature.

[me4502]

The problem is the case you’ve presented doesn’t do anything different than the existing API. As-is, this isn’t proper API, but a hack specifically to just add your feature you want directly to the API instead of using the API to do it. If you can clearly outline the issue we can work on a proper API sure, but as-is this does not make sense.

A third item list is not needed, because from the context of this event it doesn’t make sense to have one. This event either handles the items, or it doesn’t. Having a third item list doesn’t make sense because there’s nothing different to do with the items.

[BlvckBytes]

If I could have used the existing API to do it, I would've; I'm not trying to make it hard on myself, nor is it my intention to waste your time.

What highlights the additional list of items is that they are not permanently removed from the pipe, but added back in, after filteredItems (not affected by the event!) has been removed. I will admit that the solution is not ideal; it however does "solve" the problem of rejecting items from being put into the container while still keeping them in the pipe.

My requirement towards the API is really straightforward: I need to override the severely limited standard filters and execute my own predicate on the list of items currently in the pipe, in order to generate a custom filteredItems; I do not want to touch any other part of the mechanic.

With the above in mind, I'd like to propose a better solution than what I came up with initially. How about creating a PipeFilterEvent which outsources filtering if a consumer took action and falls back to internal standard measures otherwise? I will add another commit to my PR in order to undo my additions to the PipePutEvent and make the aforementioned changes.

[me4502]

My point around the existing API is that the question/changes fundamentally make no sense. Given you're handling everything already in the handler, there's no need for a third state because nothing different can happen. Items remain in the pipe if they are not handled, that's how the event works. Either there's something that's missing from what you're explaining, or there seems to be a fundamental misunderstanding of how pipes work.

The current state of the PR appears to basically just add a hook for your plugin, I'm not sure if I'm really comfortable having that (especially not as the primary side of the conditional) due to the added maintenance burden. If you did want to add proper API here rather than just a hook for your plugin, then i'm open for discussion- but as-is it seems to really just add extra maintenance burden specifically to benefit your own server. At that point you may as well just maintain a fork with a single commit that adds your functionality.

If you did want to make proper API, it'd make sense to do it in place of the ItemUtil.filterItems call. Turning that into an event that exposes the original items, inclusions, exclusions, and a way to set the list of filtered items (which by default would be the output of ItemUtil.filterItems). at least then the API would fit into the plugin and be potentially useful for third party plugins that add custom forms of items that might need alternate filtering logic.

[BlvckBytes]

Items remain in the pipe if they are not handled, that's how the event works.

They not only remain in the pipe, but are also added to the container, so I face the dichotomy of either not taking any action at all and thereby cannot employ my own additional filtering logic, or removing the item on mismatch, which then assumes that I have "handled" it and removes it from the pipe, making it disappear. Please excuse my perplexity, but I really don't understand how you can claim that "there's no need for a third state because nothing different can happen". Then again, I admit that this solution was rather specific to my use-case, as it modified the event in a way it was not meant to be used.

If you did want to make proper API, it'd make sense to do it in place of the ItemUtil.filterItems call.

Have you had a chance to look at my latest commit inside the PR? I did just that.

Turning that into an event that exposes the original items, inclusions, exclusions, and a way to set the list of filtered items (which by default would be the output of ItemUtil.filterItems).

The reason for why I didn't add inclusions and exclusions to the new event is because I wanted to completely bypass executing internal filtering logic if a consumer applied their own predicate to the pipe-contents, as to avoid needless parsing and thereby improve performance. The way I implemented it, there's no additional cost, except instantiating and dispatching the event. If you think it makes more sense to always parse the sign and add this information to the event, I'm happy to adapt my proposal.

[me4502]

The point of that existing event is that you control what you do with it, just don't put it in the container if you don't want it there? that doesn't add an extra state?

and yes, that last comment was around the current state of the PR. the current state of the PR is basically a very very specific hook for your plugin, whereas what i proposed would at least be more generic and potentially useful for other plugins. eg if a plugin added some form of custom item and wanted to allow it to be sorted in pipes in some way.

i'm also in general not happy with the concept of "has been set"; the event state should always be used, and the "default" non-modified state should correspond to existing behaviour

[BlvckBytes]

Makes sense - sorry for having been so hyper-focused on my exact use-case. The PR should now align with your remarks.

[me4502]

while filters/exceptions are the names used in code, from an API perspective they are kinda awful names. would you be able to please replace it with something like getIncludeFilters and getExcludeFilters?

[BlvckBytes]

Is there anything else that needs alteration in order for this API to become part of the project?