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

🏗 Request builder for Item Properties Setup #15

Merged
merged 2 commits into from
Nov 22, 2017

Conversation

OndraM
Copy link
Member

@OndraM OndraM commented Nov 20, 2017

This is finally the first part of the facade layer, which will be used by the end application.

(Names of the builder methods may change later - any suggestions?).

Example usage for this builder and builder factory (this is from the end application point of view):

$matej = new Matej('foo', 'apikey');

$response = $matej->request() // this actually returns the builder factory (method is not yet implemented)
    ->itemPropertiesSetup() // this is method of the builder factory, which returns the concrete builder
    ->addProperties([ItemPropertySetup::timestamp('valid_from'), ItemPropertySetup::string('title')]) // example of add multiple commands at once
    ->addProperty(ItemPropertySetup::timestamp('valid_to')) // add just one command
    ->requestWillDelete() // when this method is added, the requets will actually drop the item properties passed via addProperty(ies) method from the database
    ->send();

Thoughts, comments? PR with builder for Event endpoint will follow soon.

$this->requestManager = $requestManager;
}

public function itemPropertiesSetup(): ItemPropertiesSetupRequestBuilder
Copy link
Member

Choose a reason for hiding this comment

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

itemPropertiesSetupBuilder?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its already inside a BuilderFactory, so this would be imho not necessary "noise" in the method naming...

Also we're trying to make the builder API as much readable as possible, so not naming the class names in a full name (but using "natural language" aliases) is kind of the goal.

Does it make sense :-)?

@OndraM OndraM mentioned this pull request Nov 21, 2017
@foglcz
Copy link
Contributor

foglcz commented Nov 21, 2017

I like it, although I'm iffy about the requestWillDelete() call. The developer intent is to either create/update, or delete item properties in Matej. I'm curious whether we can make this more obvious.

Two approaches come to mind. One would be to use different factory function:

// Create or update properties:
$response = $matej->request() 
    ->createOrUpdateItemProperties() // creates ItemPropertiesSetupRequestBuilder with "POST" flag
    // ...
    ->send();

// Delete properties:
$response = $matej->request() 
    ->deleteItemProperties() // creates ItemPropertiesSetupRequestBuilder with "DELETE" flag
    // ...
    ->send();

Second approach could be changing the final send() to indicate REST-ness of the API:

$matej = new Matej('foo', 'apikey');

// Create or update item properties:
$response = $matej->request() 
    ->itemPropertiesSetup() 
    // ...
    ->post();

// Delete properties
$response = $matej->request() 
    ->itemPropertiesSetup() 
    // ...
    ->delete();

The beauty of that would be, that in future we could easily expose any PUT or GET functions of the api, by simply calling ->get() or ->put() at the end of the request build.
However, we would have to check for invalid state - so that dev cannot execute a GET request against POST only endpoint. While neat, I'm not sure if there are any GET or PUT endpoints in the pipeline - I'm not aware that there are.

Yay or nay?

@foglcz
Copy link
Contributor

foglcz commented Nov 21, 2017

Add - third option - rename $matej->request():

$matej = new Matej('foo', 'apikey');

// Create or update item properties:
$response = $matej->post() 
    ->itemPropertiesSetup() 
    // ...
    ->send();

// Delete properties
$response = $matej->delete() 
    ->itemPropertiesSetup() 
    // ...
    ->send();

I'm not sure how these affect readability of other calls.


private function assertRequestManagerIsAvailable(): void
{
if ($this->requestManager === null) {
Copy link
Contributor

@foglcz foglcz Nov 21, 2017

Choose a reason for hiding this comment

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

How would you feel about if (!$this->requestManager instanceof RequestManager) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't feel much difference here. Maybe prefer this version a little bit, as I read it like "there is no requeste manager", while the instanceof version is more like "request manager is request manager".

What about you :)? Any advantage for the instanceof version?

jirinovak
jirinovak previously approved these changes Nov 21, 2017
MarketaSebkova
MarketaSebkova previously approved these changes Nov 21, 2017
@OndraM
Copy link
Member Author

OndraM commented Nov 21, 2017

@foglcz Great ideas, thanks! I agree with the point, and I like the custom factory approach the most. I will update the PR soon :).

(BTW there is no update for ItemPropertiesSetup endpoint - once created, the properties (= "columns" in Matej database) cannot be altered. To change their data type you must first delete and then recreate them.)

So maybe this methods of the factory builder?

  • setupItemProperties()
  • deleteItemProperties()

@OndraM
Copy link
Member Author

OndraM commented Nov 21, 2017

Ok, PR updated, how do everyone like the API now? cc @jirinovak @foglcz etc.

$matej = new Matej('foo', 'apikey');

// Create new:
$response = $matej->request()
    ->setupItemProperties()
    ->addProperty(ItemPropertySetup::timestamp('valid_to'))
    ->addProperty(ItemPropertySetup::string('title'))
    ->send();

// Delete:
$response = $matej->request()
    ->deleteItemProperties()
    ->addProperty(ItemPropertySetup::timestamp('valid_to'))
    ->addProperty(ItemPropertySetup::string('title'))
    ->send();

davidkmenta
davidkmenta previously approved these changes Nov 22, 2017
@foglcz
Copy link
Contributor

foglcz commented Nov 22, 2017

Nice one! :)

foglcz
foglcz previously approved these changes Nov 22, 2017
@OndraM OndraM merged commit 51a1b83 into lmc-eu:master Nov 22, 2017
@OndraM OndraM added this to the 0.9 milestone Nov 23, 2017
@OndraM OndraM deleted the feature/request-builder branch January 15, 2024 13:27
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.

5 participants