Skip to content
This repository has been archived by the owner on Apr 22, 2020. It is now read-only.

PersistentCache: Support optional default #41

Merged
merged 2 commits into from
Aug 29, 2017

Conversation

tigrawap
Copy link
Contributor

No description provided.

@tigrawap tigrawap requested a review from koreno August 29, 2017 12:42
Copy link
Contributor

@koreno koreno left a comment

Choose a reason for hiding this comment

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

Great opportunity to add a minimal unittest for PersistentCache.

@@ -22,6 +22,7 @@
_disable_persistance = bool(os.getenv("bamboo_planKey", os.getenv("WEKA_job_key", None))) # disable this feature when running from bamboo


_NONE = object()
Copy link
Contributor

Choose a reason for hiding this comment

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

use an easpy token, that's what they're for.
from easypy.tokens import NO_DEFAULT

unless there's some circular import... but there shouldn't be...

Copy link
Contributor Author

@tigrawap tigrawap Aug 29, 2017

Choose a reason for hiding this comment

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

can't agree with this :/

One thing when you really need to pass token between parts of app - than well, maybe.
Another thing is when it's such case.... Absolutely unnecessary overhead and complexity.

"Use it since we have it" - this is the only reason and it's not good enough

Why would I want to worry, that someone passes this token as default kwarg, due to some logic in app, when this parameter was passed from some function signature, where default was NO_DEFAULT?

@tigrawap tigrawap force-pushed the tigra/persistent-cache-default branch 3 times, most recently from 064328b to c16f1f8 Compare August 29, 2017 15:00
@tigrawap tigrawap force-pushed the tigra/persistent-cache-default branch from c16f1f8 to 415df91 Compare August 29, 2017 15:04
@koreno koreno merged commit 5626bd1 into master Aug 29, 2017
@YuvalEvron YuvalEvron deleted the tigra/persistent-cache-default branch August 6, 2019 08:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants