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

Follow-up aggregators implementation #445

Merged
merged 2 commits into from
Aug 16, 2021

Conversation

mdumandag
Copy link
Contributor

There were couple of missing things in the community PR for the
aggregators. This is an attempt to fix those. Namely,

  • Some missing aggreagators are added (distinct, min_by, max_by).
    For them, some identified factories are also added, as they
    have different return values.
  • Aggregator module is added to the API documentation
  • A code sample is added
  • Docstrings are updated
  • Documentation is updated and corrected

@codecov-commenter
Copy link

Codecov Report

Merging #445 (92b7b99) into master (603a17d) will decrease coverage by 0.14%.
The diff coverage is 94.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
- Coverage   86.09%   85.95%   -0.15%     
==========================================
  Files         340      340              
  Lines       17850    17915      +65     
==========================================
+ Hits        15368    15398      +30     
- Misses       2482     2517      +35     
Impacted Files Coverage Δ
hazelcast/serialization/objects.py 87.27% <76.92%> (-9.28%) ⬇️
hazelcast/aggregator.py 97.87% <100.00%> (+0.48%) ⬆️
hazelcast/core.py 91.78% <100.00%> (+0.66%) ⬆️
hazelcast/serialization/service.py 64.42% <100.00%> (-1.27%) ⬇️
hazelcast/reactor.py 77.51% <0.00%> (-6.70%) ⬇️
hazelcast/proxy/base.py 88.67% <0.00%> (-0.63%) ⬇️
hazelcast/connection.py 82.54% <0.00%> (-0.19%) ⬇️
hazelcast/statistics.py 41.80% <0.00%> (+0.33%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 603a17d...92b7b99. Read the comment docs.

There were couple of missing things in the community PR for the
aggregators. This is an attempt to fix those. Namely,

- Some missing aggreagators are added (distinct, min_by, max_by).
  For them, some identified factories are also added, as they
  have different return values.
- Aggregator module is added to the API documentation
- A code sample is added
- Docstrings are updated
- Documentation is updated and corrected
Copy link
Member

@srknzl srknzl left a comment

Choose a reason for hiding this comment

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

LGTM, left some minor comments

hazelcast/aggregator.py Show resolved Hide resolved
hazelcast/aggregator.py Show resolved Hide resolved
hazelcast/aggregator.py Outdated Show resolved Hide resolved
hazelcast/aggregator.py Outdated Show resolved Hide resolved
hazelcast/aggregator.py Show resolved Hide resolved
tests/integration/backward_compatible/proxy/map_test.py Outdated Show resolved Hide resolved
@mdumandag mdumandag merged commit f3d2f25 into hazelcast:master Aug 16, 2021
@mdumandag mdumandag deleted the aggregator-followup branch August 16, 2021 12:37
@mdumandag mdumandag mentioned this pull request Aug 18, 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