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

Switch to functools.cached_properties (#1796) #1843

Merged
merged 1 commit into from
Jun 12, 2020

Conversation

jessebrennan
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the orange [process] Done by the Azul team label May 28, 2020
@jessebrennan jessebrennan force-pushed the issues/jessebrennan/1796-cached_property branch 6 times, most recently from 4f6ba37 to 2421f66 Compare June 1, 2020 22:01
@codecov
Copy link

codecov bot commented Jun 1, 2020

Codecov Report

Merging #1843 into develop will decrease coverage by 0.00%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1843      +/-   ##
===========================================
- Coverage    83.80%   83.79%   -0.01%     
===========================================
  Files          110      110              
  Lines        10523    10518       -5     
===========================================
- Hits          8819     8814       -5     
  Misses        1704     1704              
Impacted Files Coverage Δ
src/azul/lambda_layer.py 0.00% <0.00%> (ø)
src/azul/queues.py 0.00% <0.00%> (ø)
test/integration_test.py 0.00% <0.00%> (ø)
src/azul/health.py 96.00% <71.42%> (-0.07%) ⬇️
src/azul/azulclient.py 35.86% <100.00%> (-0.44%) ⬇️
src/azul/deployment.py 60.38% <100.00%> (-0.26%) ⬇️
src/azul/indexer/document_service.py 87.87% <100.00%> (-0.36%) ⬇️
src/azul/indexer/index_controller.py 53.07% <100.00%> (ø)
src/azul/plugins/repository/dss/__init__.py 55.55% <100.00%> (ø)
src/azul/service/elasticsearch_service.py 81.49% <100.00%> (ø)
... and 3 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 dd3871d...9a24886. Read the comment docs.

@coveralls
Copy link

coveralls commented Jun 1, 2020

Coverage Status

Coverage decreased (-0.007%) to 83.677% when pulling 9a24886 on issues/jessebrennan/1796-cached_property into dd3871d on develop.

@jessebrennan jessebrennan requested review from hannes-ucsc and amarjandu and removed request for hannes-ucsc June 1, 2020 22:25
Copy link
Contributor

@amarjandu amarjandu left a comment

Choose a reason for hiding this comment

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

LGTM.

@amarjandu amarjandu removed their assignment Jun 1, 2020
@jessebrennan jessebrennan requested a review from hannes-ucsc June 1, 2020 22:56
@@ -64,3 +64,17 @@ def __new__(_cls, *args, **kwargs):
return _self

cls.__new__ = __new__


# Vendored from boltons==19.3.0
Copy link
Member

Choose a reason for hiding this comment

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

I would call this theft, not vendoring.

Suggested change
# Vendored from boltons==19.3.0
# Stolen from boltons==19.3.0



# Vendored from boltons==19.3.0
class classproperty(object):
Copy link
Member

Choose a reason for hiding this comment

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

Does PyCharm infer the return type correctly with this?

Does this technique have any advantages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does PyCharm infer the return type correctly with this?

No, not for my test:

class C:
    @classproperty
    def foo(cls) -> int:
        return 42
    

c = "a" + C.foo
d = "a" + 42

Does this technique have any advantages?

PyCharm does not correctly infer types with this form either. Since this form requires two decorators, I think it is inferior. Also the two decorators raise a different, "incompatible decorator" warning in pycharm.

This implementation is functionally the same as the boltons one, but with better type annotations. Even though the type annotations don't allow inference with PyCharm, I'll steal this one instead.

Copy link
Member

Choose a reason for hiding this comment

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

The main reason we switch to functools.cached_property is to enable PyCharm type inference.

I think we should not encourage the use of @classproperty since there is no implementation that we know of that allows PyCharm type inference of the return type. For that reason, I think we should completely remove the decorator and eliminate the one use in health.py.

@hannes-ucsc hannes-ucsc removed their assignment Jun 2, 2020
@jessebrennan jessebrennan requested a review from hannes-ucsc June 2, 2020 20:04
Copy link
Member

@hannes-ucsc hannes-ucsc left a comment

Choose a reason for hiding this comment

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

@hannes-ucsc hannes-ucsc added the 0 reviews [process] Lead didn't request any changes label Jun 2, 2020
@hannes-ucsc hannes-ucsc removed their assignment Jun 2, 2020
@jessebrennan jessebrennan force-pushed the issues/jessebrennan/1796-cached_property branch from 645bc2c to 378dce2 Compare June 3, 2020 16:31
@jessebrennan jessebrennan requested a review from hannes-ucsc June 3, 2020 17:35
Copy link
Member

@hannes-ucsc hannes-ucsc left a comment

Choose a reason for hiding this comment

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

Clean work.

@hannes-ucsc hannes-ucsc added the sandbox [process] Resolution is being verified in sandbox deployment label Jun 4, 2020
@hannes-ucsc hannes-ucsc force-pushed the issues/jessebrennan/1796-cached_property branch from 378dce2 to 22d4171 Compare June 4, 2020 21:36
Copy link
Member

@hannes-ucsc hannes-ucsc left a comment

Choose a reason for hiding this comment

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

I guess I have to take that back. IT fails.

@hannes-ucsc hannes-ucsc added 1 review [process] Lead requested changes once and removed 0 reviews [process] Lead didn't request any changes sandbox [process] Resolution is being verified in sandbox deployment labels Jun 4, 2020
@hannes-ucsc hannes-ucsc removed their assignment Jun 4, 2020
@jessebrennan jessebrennan force-pushed the issues/jessebrennan/1796-cached_property branch from 22d4171 to 91745a9 Compare June 5, 2020 21:23
@jessebrennan jessebrennan requested a review from hannes-ucsc June 5, 2020 23:37
@hannes-ucsc hannes-ucsc added the hold warm [process] PR can't land just yet but needs to be rebased daily label Jun 11, 2020
Copy link
Member

@hannes-ucsc hannes-ucsc left a comment

Choose a reason for hiding this comment

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

History was borked again.

image

I fixed the history, resolved conflicts and ran make requirements_upgrade.

@hannes-ucsc hannes-ucsc force-pushed the issues/jessebrennan/1796-cached_property branch from 91745a9 to 9a24886 Compare June 12, 2020 17:41
@hannes-ucsc hannes-ucsc added sandbox [process] Resolution is being verified in sandbox deployment 2 reviews [process] Lead requested changes twice and removed hold warm [process] PR can't land just yet but needs to be rebased daily 1 review [process] Lead requested changes once labels Jun 12, 2020
@hannes-ucsc
Copy link
Member

History was borked again.

Bumping review count for that reason.

@hannes-ucsc hannes-ucsc merged commit 3faa455 into develop Jun 12, 2020
@hannes-ucsc hannes-ucsc deleted the issues/jessebrennan/1796-cached_property branch June 12, 2020 19:21
@hannes-ucsc hannes-ucsc removed their assignment Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 reviews [process] Lead requested changes twice orange [process] Done by the Azul team sandbox [process] Resolution is being verified in sandbox deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants