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

Allow to process search results. Refs #75. #76

Merged
merged 3 commits into from
Jul 10, 2015
Merged

Conversation

hajoeichler
Copy link
Member

@emmenko please review

@@ -250,7 +250,6 @@ class ProductProjectionService extends BaseService
# .search()
search: ->
@_currentEndpoint = '/product-projections/search'
@fetch()
Copy link
Member

Choose a reason for hiding this comment

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

I don't like it, it's a breaking change. We should think of something else.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a matter of documentation ;)

See #75

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, the endpoint is set on the fly...let me have another look then ;)

Copy link
Member

Choose a reason for hiding this comment

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

I guess the painless solution would be to have a new method called processSearch. Any other solution would be a breaking change I guess.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm ok, well just for search then, so using forSearch.

Copy link
Member Author

Choose a reason for hiding this comment

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

useSearch maybe?

Copy link
Member

Choose a reason for hiding this comment

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

You're using the service for search. Or asSearch.

Copy link
Member Author

Choose a reason for hiding this comment

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

asSearch FTW!

Copy link
Member

Choose a reason for hiding this comment

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

:)

@hajoeichler
Copy link
Member Author

Will adapt to asSearch as extra method.

@hajoeichler
Copy link
Member Author

@emmenko please check again.

# .asSearch()
# .process(...)
asSearch: ->
@_currentEndpoint = '/product-projections/search'
Copy link
Member

Choose a reason for hiding this comment

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

You have to return this

@emmenko
Copy link
Member

emmenko commented Jul 10, 2015

Add a test and squash your commits then before merging, thanks

hajoeichler added a commit that referenced this pull request Jul 10, 2015
Allow to process search results. Refs #75.
@hajoeichler hajoeichler merged commit 7ae8e37 into master Jul 10, 2015
@hajoeichler hajoeichler deleted the process-all branch July 10, 2015 18:04
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

2 participants