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 objects too coupled to TAP #61

Open
Leont opened this issue Nov 19, 2015 · 5 comments
Open

Event objects too coupled to TAP #61

Leont opened this issue Nov 19, 2015 · 5 comments

Comments

@Leont
Copy link

Leont commented Nov 19, 2015

Currently, Event objects are themselves responsible for turning themselves into TAP. For proper decoupling, this really belongs in the Formatter class. Essentially all of it.

@exodist
Copy link
Member

exodist commented Nov 19, 2015

The problem there is if you want to write a new event type that has a TAP form, do you also subclass the formatter, what if there are 2 such event types you wish to add?

I actually did make the change you are suggesting in a branch, I had some significant performance issues as a result, but I don't remember exactly why, I will have to re-produce my results and share them.

I think it boiled down to making it so that the formatter had a registration system allowing you to register to_tap converters for custom event types. Ultimately I decided allowing objects to define a to_tap method was cleaner and did the same thing using the language built-ins and the actual benefits of OO instead of re-engineering it all.

I am open to changing this, but I want more discussion first.

@kentfredric
Copy link

Personally based on what is written here, I don't think Event objects should be able to define themselves in terms of TAP. That's basically a guarantee that every new event will be its own special snowflake which will become useless if somebody ever wants a target other than TAP.

Event objects should/could be required to represent themselves in terms of other events in a shared base namespace, and its then the job of the formatter to translate these event series to TAP.

Unless of course you want to define TAP to be the "Shared namespace" that everything must be represented in, and then other non-TAP targets being forced to go through TAP first.

At best, I'm leaning towards Events being able to TAP-ify themselves to be an optimisation akin to Mooses hand-written pre-compiled type constraints.

But I haven't read quite enough source to know what I'm talking about here, this is just my eyeballing from the given context.

@Leont
Copy link
Author

Leont commented Nov 20, 2015

I actually did make the change you are suggesting in a branch, I had some significant performance issues as a result, but I don't remember exactly why, I will have to re-produce my results and share them.

You'd probably want to optimize for the common case of Event::Ok objects, anything else isn't likely to be performance sensitive anyway.

I think it boiled down to making it so that the formatter had a registration system allowing you to register to_tap converters for custom event types. Ultimately I decided allowing objects to define a to_tap method was cleaner and did the same thing using the language built-ins and the actual benefits of OO instead of re-engineering it all.

This quickly becomes tricky when there's a new version of TAP, or when one wants to use something other than TAP (

@exodist
Copy link
Member

exodist commented Nov 20, 2015

Sorry, I meant that the TAP formatter specifically had a registration system.

I may try again without such a system and say that custom events cannot produce tap without writing a custom formatter (or subclass of the TAP formatter), but that has its own set of problems...

@rjbs
Copy link

rjbs commented Dec 11, 2015

Updated after weekly status meeting today: @exodist says this is probably sorted out by #64. How does that look to you, @Leont?

I'd like to make sure we're all 👍 on not only the specific changes, but the model they imply, for the sake of forward compatibility.

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

No branches or pull requests

4 participants