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

Issue52 : First method change for approuval #70

Merged
merged 5 commits into from
Feb 2, 2016
Merged

Conversation

emmurd
Copy link
Contributor

@emmurd emmurd commented Feb 2, 2016

No description provided.

@mat128
Copy link
Contributor

mat128 commented Feb 2, 2016

Good job tackling #52 =)

With all the tests using the new method name, I'm worried that using remove_access_vlan does not behave the way unset_access_vlan does. If you remove remove_access_vlan from switch base, I'm sure all the tests still pass. Also, I would like to know that a certain call is deprecated when I call it. Maybe a log message or something?

@idjaw
Copy link
Contributor

idjaw commented Feb 2, 2016

LGTM. Once conflicts are resolved, will merge.

# Conflicts:
#	netman/core/objects/switch_transactional.py
from netman.core.objects.switch_base import SwitchOperations


class BackwardCompatiblieSwitchOperationsTest(TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo. I think it should be: BackwardsCompatibleSwitchOperationsTest

With that fix, also make sure the filename reflects the name to: backwards_compatible_switch_operations_test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oups!! Sorry! Fixing this right away!

@lindycoder
Copy link
Contributor

👍

idjaw added a commit that referenced this pull request Feb 2, 2016
Issue52 : 	First method change for approuval
@idjaw idjaw merged commit 809d092 into internap:master Feb 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants