Adds doc string and validation for mirror parameter #3583
Conversation
|
Can one of the admins verify this patch? |
| if sync_mode != 'mirror' and sync_mode != 'additive': | ||
| msg = _("'sync_mode' must either be 'mirror' or 'additive' not '{sync_mode}'") | ||
| raise ValueError(msg.format(sync_mode=sync_mode)) | ||
| assert(sync_mode == 'mirror' or sync_mode == 'additive') |
There was a problem hiding this comment.
I'm not sure I understand this change?
There was a problem hiding this comment.
Ah, I see now. The value is being validated by the serializer.
There was a problem hiding this comment.
I think a ValueError is more correct than an AssertionError here. Look at the description of ValueError in the standard library docs. https://docs.python.org/3/library/exceptions.html#ValueError
There was a problem hiding this comment.
This was based on my suggestion so I'm to blame ;) The case will be more obvious if proper constants and namespacing are used:
class DeclarativeVersion:
sync_mode_mirror = 'mirror'
sync_mode_additive = 'additive'
default_sync_mode = sync_mode_mirror
def __init__(self, first_stage, repository, sync_mode=None):
...
if sync_mode is None:
sync_mode = self.default_sync_mode
assert sync_mode in {self.sync_mode_additive, sync_mode_mirror}, 'Oops!'IMO a declarative_version object always expects just the values/switches it was declared with therefore the assert statement is just a debugging thing.
| @@ -12,7 +11,7 @@ | |||
|
|
|||
| class DeclarativeVersion: | |||
|
|
|||
| def __init__(self, first_stage, repository, sync_mode='mirror'): | |||
| def __init__(self, first_stage, repository, sync_mode): | |||
There was a problem hiding this comment.
Do we want plugin writers to not have a default value for sync_mode?
Also if sync_mode isn't getting a default there are various docstrings that need updating.
There was a problem hiding this comment.
I deleted it because it seemed to me that the default value is assigned twice. Once here and once in the file plugin. But I didn't realize that there could be plugins that might not do that. So I made the changes back. Thanks.
Codecov Report
@@ Coverage Diff @@
## master #3583 +/- ##
==========================================
+ Coverage 57.7% 57.72% +0.01%
==========================================
Files 60 60
Lines 2523 2524 +1
==========================================
+ Hits 1456 1457 +1
Misses 1067 1067
Continue to review full report at Codecov.
|
|
FYI, there's a new thread on sync modes on pulp-dev that we may want to wait on before merging this PR. https://www.redhat.com/archives/pulp-dev/2018-August/msg00035.html |
| if sync_mode != 'mirror' and sync_mode != 'additive': | ||
| msg = _("'sync_mode' must either be 'mirror' or 'additive' not '{sync_mode}'") | ||
| raise ValueError(msg.format(sync_mode=sync_mode)) | ||
| assert(sync_mode == 'mirror' or sync_mode == 'additive') |
There was a problem hiding this comment.
I don't think we should use assert here. Instead we should raise a ValueError which is already there. I think this file should be left unedited.
|
I responded to the mailing list thread about ditching sync modes. I'm going to wait until Monday and if there's no response, let's merge this PR. |
|
@daviddavis +1 |
vdusek
changed the title
re # 3494
https://pulp.plan.io/issues/3494
closes #3944
https://pulp.plan.io/issues/3944