Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

Unit test for delete_organization and if statement #28

Merged
merged 1 commit into from
Jun 5, 2015

Conversation

vubo
Copy link
Contributor

@vubo vubo commented Jun 4, 2015

PR to the develop branch

@vubo vubo mentioned this pull request Jun 4, 2015
5 tasks
@willingc
Copy link
Contributor

willingc commented Jun 4, 2015

@vubo I haven't had a chance to test locally yet. So a couple of suggestions, add a docstring for the function. The docstring can be a simple one line or a more formal multi-line - your choice. Run the code through a pep8 and see if there are any issues. Normally, we would have Travis do this so in the meanwhile I would appreciate if you can do locally. You may also wish to check out pylint or flake8.

I am wondering and I haven't come up with an answer as to whether the functionality to check if an organization is empty should be inside the delete function or outside of it. It would be good to somehow guard against accidental deletion if the code fails silently, and I'm not sure that there is enough of a guard in place now. That said, we can always refactor later. Spend a few minutes thinking on this and let me know what you think :)

@vubo
Copy link
Contributor Author

vubo commented Jun 4, 2015

@willingc do you mean for example organization_empty function, that we can use then everywhere?

@willingc
Copy link
Contributor

willingc commented Jun 5, 2015

@vubo Yeah, I'm wondering if we should refactor the function so that a function that checks if the organization is empty could be called within the delete function.

I'm going to go ahead and merge this since it tests cleanly. Would you mind opening an issue (referencing this PR) to refactor the delete_organization function so that we keep track of it as a future action item?

Thanks for submitting this PR.

willingc added a commit that referenced this pull request Jun 5, 2015
Add unit test for delete_organization and if statement
Addresses one task in issue #29
@willingc willingc merged commit 229c426 into anitab-org:develop Jun 5, 2015
@vubo
Copy link
Contributor Author

vubo commented Jun 5, 2015

@willingc Thank you, I opened the issue and will refactor the function.

This was referenced Jun 7, 2015
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