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.

Sign up for GitHub

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

Jump to bottom

Add "missing" behavior to some aggregations #1875

Merged
merged 1 commit into from
Dec 7, 2020
Merged

Add "missing" behavior to some aggregations #1875

merged 1 commit into from
Dec 7, 2020

Conversation

romainneutron
Copy link
Contributor

No description provided.

@romainneutron romainneutron force-pushed the add-missing branch 3 times, most recently from 7046c60 to ab9850a Compare November 23, 2020 17:23
@deguif deguif mentioned this pull request Nov 23, 2020
Closed
@romainneutron romainneutron force-pushed the add-missing branch 2 times, most recently from 86ba780 to a3b8ca9 Compare November 23, 2020 18:16
deguif
deguif reviewed Nov 24, 2020
View reviewed changes
Copy link
Collaborator

@deguif deguif left a comment

Choose a reason for hiding this comment

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

Another 👍 for using trait.

docker/docker-compose.es75.yml Outdated Show resolved Hide resolved
src/Aggregation/Cardinality.php Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/Aggregation/Percentiles.php Show resolved Hide resolved
@romainneutron romainneutron force-pushed the add-missing branch 2 times, most recently from 178ec3d to ef1b8e5 Compare November 24, 2020 12:34
@romainneutron
Copy link
Contributor Author

PR updated comments addressed

ruflin
ruflin reviewed Nov 24, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
ruflin
ruflin approved these changes Nov 25, 2020
View reviewed changes
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.

LGTM. Sorry about the changelog. I leave it to you which option you prefer.

@deguif
Copy link
Collaborator

deguif commented Nov 25, 2020

Some conflicts to resolve, and we should be able to merge it.

@romainneutron
Copy link
Contributor Author

PR rebased

@deguif
Copy link
Collaborator

deguif commented Nov 25, 2020

It seems we have issues with Travis (too much credits consumed)
ping @ruflin

Screenshot 2020-11-25 at 19 54 35

@ruflin
Copy link
Owner

ruflin commented Nov 26, 2020

:-( My initial instinct was to quickly buy some additional credits but seems the only option is a monthly subscription. Perhaps time to move to Github actions? Anyone experience with it? I only skimmed through https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/migrating-from-travis-ci-to-github-actions but at least it seems doable. We could try perhaps first the linting check and then move on to the other builds?

@deguif
Copy link
Collaborator

deguif commented Dec 3, 2020

@romainneutron sorry for the delay but we had to migrate tests on github action.
Now that is done, could you rebase your PR so that we can merge it.

@romainneutron
Copy link
Contributor Author

PR rebased

deguif
deguif reviewed Dec 7, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@deguif deguif merged commit 0167237 into ruflin:master Dec 7, 2020
@romainneutron romainneutron deleted the add-missing branch December 9, 2020 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin approved these changes

@deguif deguif deguif approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@romainneutron @deguif @ruflin