Projections follow up #454

mdumandag · 2021-08-19T15:15:59Z

This is a follow up PR after merging the Projections PR. This is
a combination of lots of small things.

Add documentation to properties of MapEntry so that they are
displayed
Move map#project so that we maintain alphabetical order
Add some corner case tests for projections and aggregations
Used asserCountEqual on projection tests so that the tests will
be more durable, even if we add more items to map
Add missing API documentation for projections
Fix API documentation of projections around the return value documentations
Add unit tests for the invalid projection inputs
Make the projections code snippet simpler
Add a code sample for projections

This is a follow up PR after merging the Projections PR. This is a combination of lots of small things. - Add documentation to properties of `MapEntry` so that they are displayed - Move map#project so that we maintain alphabetical order - Add some corner case tests for projections and aggregations - Used asserCountEqual on projection tests so that the tests will be more durable, even if we add more items to map - Add missing API documentation for projections - Fix API documentation of projections around the return value documentations - Add unit tests for the invalid projection inputs - Make the projections code snippet simpler - Add a code sample for projections

codecov-commenter · 2021-08-19T15:34:50Z

Codecov Report

Merging #454 (6dcc95f) into master (f05112e) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #454      +/-   ##
==========================================
- Coverage   94.45%   94.21%   -0.24%     
==========================================
  Files         345      345              
  Lines       17545    17545              
==========================================
- Hits        16572    16530      -42     
- Misses        973     1015      +42

Impacted Files	Coverage Δ
hazelcast/core.py	`94.48% <ø> (ø)`
hazelcast/projection.py	`93.75% <100.00%> (+6.25%)`	⬆️
hazelcast/proxy/map.py	`89.17% <100.00%> (+0.34%)`	⬆️
...otocol/codec/client_authentication_custom_codec.py	`50.00% <0.00%> (-23.81%)`	⬇️
hazelcast/reactor.py	`79.56% <0.00%> (-7.55%)`	⬇️
hazelcast/connection.py	`90.84% <0.00%> (-1.47%)`	⬇️
hazelcast/invocation.py	`92.72% <0.00%> (-0.39%)`	⬇️
hazelcast/statistics.py	`88.62% <0.00%> (+0.59%)`	⬆️
hazelcast/proxy/base.py	`98.05% <0.00%> (+0.64%)`	⬆️
hazelcast/partition.py	`90.78% <0.00%> (+1.31%)`	⬆️
... 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 f05112e...6dcc95f. Read the comment docs.

yuce

Looks good, but I think using attribute instead of single_attribute and multi_attribute would make our API simpler and more Pythonic.

yuce · 2021-08-20T07:50:14Z

docs/using_python_client_with_hazelcast_imdg.rst

+ filtered_ages = employees.project(single_attribute("age"), greater("age", 23))
+
+ # Prints: "Ages of the filtered employees are [25, 40]"
+ print("Ages of the filtered employees are %s" % filtered_ages)
+
+ attributes = employees.project(multi_attribute("age", "height"))


How about replacing single_attribute and multi_attribute with just attribute?

But they are two distinct things with distinct return types. One returns list[any], the other list[list[any]] when used with project method.

Also, I find such codes like the below a bit weird in the implementation. IMHO, a separation is better than this

def attribute(*attrs): if len(attrs) == 1: return _SingleAttribute(attrs[0]) return _MultiAttribute(attrs)

If we make it just attribute, the return type can be list[list[any]]. I think it's not weird at all to distinguish between _SingleAttribute and _MultiAttribute depending on the number of attributes. Those two are implementation details, the user doesn't need to know about them. (I didn't check that, but I guess they are different in the protocol as well, not sure about the reason of that decision)

yuce · 2021-08-20T08:11:47Z

hazelcast/projection.py

@@ -48,7 +48,7 @@ def get_class_id(self):


 class _MultiAttributeProjection(_AbstractProjection):
- def __init__(self, *attribute_paths):
+ def __init__(self, attribute_paths):


How about adding types to new classes and functions?

I have added type hints to the projections module. I didn't type-hint the code that is coming from the superclass (IdentifiedDataSerializable), should I add type hints to those methods too (like get_class_id)?

Also, I question. We have docstrings that define types for public methods, should we define type hints for them too? I have added type hints to them but I can remove them if you want

According to this SO discussion, it is possible to configure sphinx to recognize types, so writing in docstrings is probably not necessary: https://stackoverflow.com/questions/40071573/python-3-sphinx-doesnt-show-type-hints-correctly But I think we should keep them until we are sure about that.

IMO adding types to only functions/classes added/changed in this PR is OK. We can tackle with adding types to others in a separate PR. I think types in the base class are used for derived classes, but since IdentifiedDataSerializable is an older class, I think it's OK to not add types to get_class_id et al.

yuce · 2021-08-20T08:14:16Z

tests/integration/backward_compatible/proxy/map_test.py

+ with self.assertRaises(AssertionError):
+ self.map.aggregate(None)


Can be simplified as: self.assertRaises(AssertionError, lambda: self.map.aggregate(None)) I think most of other assertRaises uses can also be put into a single line.

I personally prefer this style, and try to use it like this across the codebase. Advantages:

You can easily put breakpoints

You can put multiple lines without defining a closure

And IMHO, this one is more readable

yuce · 2021-08-20T09:59:44Z

docs/using_python_client_with_hazelcast_imdg.rst

+ filtered_ages = employees.project(single_attribute("age"), greater("age", 23))
+
+ # Prints: "Ages of the filtered employees are [25, 40]"
+ print("Ages of the filtered employees are %s" % filtered_ages)
+
+ attributes = employees.project(multi_attribute("age", "height"))


If we make it just attribute, the return type can be list[list[any]]. I think it's not weird at all to distinguish between _SingleAttribute and _MultiAttribute depending on the number of attributes. Those two are implementation details, the user doesn't need to know about them. (I didn't check that, but I guess they are different in the protocol as well, not sure about the reason of that decision)

yuce · 2021-08-20T10:04:36Z

hazelcast/projection.py

@@ -48,7 +48,7 @@ def get_class_id(self):


 class _MultiAttributeProjection(_AbstractProjection):
- def __init__(self, *attribute_paths):
+ def __init__(self, attribute_paths):


According to this SO discussion, it is possible to configure sphinx to recognize types, so writing in docstrings is probably not necessary: https://stackoverflow.com/questions/40071573/python-3-sphinx-doesnt-show-type-hints-correctly But I think we should keep them until we are sure about that.

IMO adding types to only functions/classes added/changed in this PR is OK. We can tackle with adding types to others in a separate PR. I think types in the base class are used for derived classes, but since IdentifiedDataSerializable is an older class, I think it's OK to not add types to get_class_id et al.

mdumandag added Type: Cleanup Source: Internal labels Aug 19, 2021

mdumandag added this to the 4.2.1 milestone Aug 19, 2021

mdumandag self-assigned this Aug 19, 2021

yuce requested changes Aug 20, 2021

View reviewed changes

mdumandag added 2 commits August 20, 2021 12:46

add type hints to projections

2835336

add projections to feature list

6dcc95f

yuce approved these changes Aug 20, 2021

View reviewed changes

mdumandag merged commit 5603400 into hazelcast:master Aug 20, 2021

mdumandag deleted the projections branch August 20, 2021 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Projections follow up #454

Projections follow up #454

mdumandag commented Aug 19, 2021

codecov-commenter commented Aug 19, 2021 •

edited

Loading

yuce left a comment

yuce Aug 20, 2021

mdumandag Aug 20, 2021

yuce Aug 20, 2021

yuce Aug 20, 2021

mdumandag Aug 20, 2021

yuce Aug 20, 2021

yuce Aug 20, 2021

mdumandag Aug 20, 2021

yuce Aug 20, 2021

yuce Aug 20, 2021

		with self.assertRaises(AssertionError):
		self.map.aggregate(None)

Projections follow up #454

Projections follow up #454

Conversation

mdumandag commented Aug 19, 2021

codecov-commenter commented Aug 19, 2021 • edited Loading

Codecov Report

yuce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 19, 2021 •

edited

Loading