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

WIP Draft for new result format interface design #4313

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JeroenDeDauw
Copy link
Member

@JeroenDeDauw JeroenDeDauw commented Sep 25, 2019

Progress towards #125. This new interface will be an alternative to the existing one, not a replacement.

Based on work in the ModernTimeline extension.

No dedicated tests in this draft yet, though most code tested in
ModernTimeline via integration and system tests.


Design improvements over old ResultPrinter interface:

  • Constructor injection possible
  • Query result is simple data structure without global state access: testing possible
  • Query result data structure allows extending logic and data without BC breaks
  • Super simple interface without typically not needed methods/complexity
  • Distinction between result format and result printer (called presenter in new interface)

Example registration:

$resultFormatRegistry->newFormat()
	->withName( 'moderntimeline' )
	->andMessageKey( 'modern-timeline-format-name' )
	->andParameterDefinitions( TimelineOptions::getTimelineParameterDefinitions() )
	->andPresenterBuilder( function() {
		return new TimelinePresenter( $timelimeFactory->getSomeCollaborator() );
	} )
	->register();

Example implementation:

class TimelinePresenter implements ResultPresenter {
	// Example of the now possible constructor injection
	public function __construct( SomeCollaborator $collaborator ) {
	}

	// Tests can create the $result data structure
	public function presentResult( SimpleQueryResult $result ): string {
		return 'construct string from $result data structure';
	}
}

TODO:

  • Pass something for side effects (like adding JS to output) to presentResult
  • Deal with PrintRequest (global access and constructor parameters)
  • naming naming naming
  • Are parameters part of the format or the presenter?
  • unit tests
  • integration tests
  • Finish new registration system
  • Make use of printers registered via new registration system
  • (optional) Add ability to register just presenter class name for when DI is not needed
  • (optional) Refactor some existing SMW format(s) to the new system

Based on work in the ModernTimeline extension.

https://github.com/ProfessionalWiki/ModernTimeline/tree/fe1ca3e2686752ab7e67f69586b4bfac0b91b459/src/ResultFacade

No dedicated tests in this draft yet, though most code tested in
ModernTimeline via integration and system tests.
@JeroenDeDauw JeroenDeDauw changed the title WIP Design draft for new result format interface WIP Draft for new result format interface design Sep 25, 2019
@krabina
Copy link
Contributor

krabina commented Sep 25, 2019

this is somehting for @s7eph4n

@s7eph4n
Copy link
Contributor

s7eph4n commented Oct 13, 2019

I like the approach.

I am not sure though if I am a fan of that and-and-and-construct. One issue for me is that from their title it is not immediately obvious what those methods do. And generally, what's the advantage over just requiring four parameters?

@JeroenDeDauw
Copy link
Member Author

@s7eph4n this is a fluent interface which you'll have seen before in places such as PHPUnits mock builder. Positional parameter lists have the disadvantage that (a) they are positional, so swapping parameters breaks things and (b) they are not named so hard to understand without looking at the function definition. Also some advantages when dealing with optional parameter and adding new parameters. And some disadvantages in that the required parameters are less obvious. All the usual fluent interface + builder pattern stuff really.

Naming can be changed and in the end how exactly the registry takes the info is an implementation detail. I'm much more interested in what info it takes.

@JeroenDeDauw
Copy link
Member Author

@mwjames thoughts?

@kghbln
Copy link
Member

kghbln commented Jan 27, 2020

@mwjames It will be super cool if you could have a peep on this and comment. My general impression as a user is that result formats need kinda more love. This may be the way to ....

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

4 participants