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

Add possibility to specify PointInTime on Elastica\Query::setPointInTime() #1992

Merged
merged 1 commit into from
Oct 19, 2021

Conversation

thePanz
Copy link
Collaborator

@thePanz thePanz commented Oct 8, 2021

@thePanz thePanz force-pushed the add-pit-search-parameter branch from b7e3346 to 406bc04 Compare October 8, 2021 15:50
@thePanz thePanz requested review from deguif and ruflin October 11, 2021 06:58
Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Any chance to get some functional tests added?

@thePanz thePanz force-pushed the add-pit-search-parameter branch from 406bc04 to 7181c98 Compare October 17, 2021 19:39
@thePanz
Copy link
Collaborator Author

thePanz commented Oct 18, 2021

@ruflin tests (functional and unit) added :)

@thePanz thePanz requested a review from ruflin October 18, 2021 09:08
/**
* Implementation of Point-In-Time for the Search request.
*/
class PointInTime extends Param
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if at one stage we need to find a better place for these classes instead of having all of them at the top level.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, but there are more of those "request" models around at the root namespace.
What do you suggest?

@thePanz thePanz merged commit eb1badc into ruflin:master Oct 19, 2021
@thePanz thePanz deleted the add-pit-search-parameter branch October 19, 2021 19:47
@deguif deguif added this to the 7.1.2 milestone Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants