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

Add log warning for deprecated op #163

Merged
merged 3 commits into from
Jul 20, 2015
Merged

Conversation

prat0318
Copy link
Contributor

Fixes #162

@prat0318
Copy link
Contributor Author

@analogue @dnephin whenever you have time ^

@@ -215,6 +215,16 @@ def construct_params(self, request, op_kwargs):
if not remaining_param.required and remaining_param.has_default():
marshal_param(remaining_param, None, request)

def log_warning_for_deprecated_op(self):
# TODO: Move this logic to bravado_decorators
if self.operation.op_spec.get('deprecated'):
Copy link
Contributor

Choose a reason for hiding this comment

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

While functionally equivalent, specifying a default value of False here is a little more explicit and in line with the spec

if self.operation.op_spec.get('deprecated', False):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@prat0318
Copy link
Contributor Author

Add PR review suggestions. ^

prat0318 added a commit to prat0318/swagger-py that referenced this pull request Jul 18, 2015
@prat0318
Copy link
Contributor Author

Moved warning logic to a separate module

if rem_date:
message += "Removal Date: {0}".format(rem_date)

warnings.warn(message, Warning)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to be super explicit, s/Warning/DeprecationWarning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used Warning instead of DeprecationWarning, as the latter is by default turned off and hence the warning will get missed.

@analogue
Copy link
Contributor

lgtm

prat0318 added a commit that referenced this pull request Jul 20, 2015
Add log warning for deprecated op
@prat0318 prat0318 merged commit 6df3710 into Yelp:master Jul 20, 2015
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.393% when pulling 0c538aa on prat0318:162_warn_deprecated into c026a60 on Yelp:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants