Skip to content
This repository has been archived by the owner on Nov 6, 2018. It is now read-only.

Missing tests - Adding two missing tests and re-ordering existing tests. #134

Merged
merged 2 commits into from Apr 20, 2016
Merged

Missing tests - Adding two missing tests and re-ordering existing tests. #134

merged 2 commits into from Apr 20, 2016

Conversation

kdexd
Copy link
Contributor

@kdexd kdexd commented Feb 7, 2016

  • I recently started using this library for practicing implementation of some algorithms and I decided to run some tests. I saw the tests were a little bit un-ordered so it was difficult to keep track.
  • The files in directory are arranged in alphabetical order and so if the tests are too arranged in that way, it is better to analyze the code for a third person viewing it.
  • So I have ordered the tests in each file alphabetically, and on the way I discovered that two tests were not written - bogo sort, and primality test. I have added them in too.
  • I develop in Pycharm IDE by Jetbrains community, and nowadays many python users do so. This IDE generates a .idea/ directory which does not need to be on Github, so I also updated the gitignore file in the PR.
  • I do not intend to do more work in the PR unless there are issues with the current code written here. So if the checks pass and the reviewer feels it is proper, this PR is fit to be merged. 馃憤

@kdexd
Copy link
Contributor Author

kdexd commented Feb 7, 2016

I am getting some ideas for impementation of an algorithm which can be a good addition to the lib. This lib is great as far as the code is concerned, I intend to contribute and open up more PRs further. 馃槃

@kdexd
Copy link
Contributor Author

kdexd commented Feb 7, 2016

Oops, got a little AssertionError, will fix it in a commit.

@kdexd
Copy link
Contributor Author

kdexd commented Feb 7, 2016

Only Python3.2 check fails. I don't know why.
@nryoung Please review this and help me find the problem. 馃槂

@nryoung
Copy link
Owner

nryoung commented Feb 7, 2016

The reason this test is failing is because virtualenv>= 14.x no longer supports py3.2. I have fixed this issue in master. Can you rebase your branch with master? If you need help with rebasing checkout: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

@kdexd
Copy link
Contributor Author

kdexd commented Feb 8, 2016

Well, that was smooth.. now fit to be merged ! 馃憤

If I find a good algorithm to implement, I will send a new PR soon, it was pleasure using your library and working with it ! 馃槃

@kdexd
Copy link
Contributor Author

kdexd commented Feb 17, 2016

@nryoung this will get stale, please suggest workarounds 馃槃

@nryoung
Copy link
Owner

nryoung commented Feb 17, 2016

@karandesai-96 This is still on my radar to review. Don't worry about it getting stale. I will get to it soon.

@kdexd
Copy link
Contributor Author

kdexd commented Feb 17, 2016

Cool. Whenever you get to it. If there is any test remaining, please let me know -- I will push a commit in no time. 馃槂

@nryoung
Copy link
Owner

nryoung commented Mar 2, 2016

Thank you for your contribution! These changes look solid, just have one comment:

Can you squash and rebase your commits to a single commit with a short but descriptive commit message about your changes. Take a look at: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request if you need help.

@kdexd
Copy link
Contributor Author

kdexd commented Mar 2, 2016

Sure, I will do it right away.

karandesai-96 added 2 commits April 9, 2016 08:37
- Add test for BogoSort and PrimalityTest.
- Reorder tests in other files alphabetically.
- Align all files according to PEP8 guidelines.
@kdexd
Copy link
Contributor Author

kdexd commented Apr 9, 2016

@nryoung I was away for a while. I did as you said, all commits squashed in two commits, but one:

  • First commit includes change in .gitignore. I kept it separate so that it helps to track this change easily when git blame is used in future.
  • Second commit has all test files related changes.

@kdexd
Copy link
Contributor Author

kdexd commented Apr 20, 2016

@nryoung A gentle reminder 馃槃

@nryoung
Copy link
Owner

nryoung commented Apr 20, 2016

This looks good! Thank you for the hard work.

@nryoung nryoung merged commit 768cea7 into nryoung:master Apr 20, 2016
@kdexd kdexd deleted the missing_tests branch April 20, 2016 14:12
@kdexd
Copy link
Contributor Author

kdexd commented Apr 20, 2016

Thank you 馃槃

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants