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

BUG: Fixes bug, adds unittest to prevent, adds exception handling, and include CI #8

Closed
wants to merge 22 commits into from

Conversation

linwoodc3
Copy link
Contributor

This is a bug fix to something I unknowingly causes in the merge request a month ago. The CI piece is trial and error so that's why there are so many. The code changes are minimal. More importantly, it fixes a bug likely causes by me and makes it so no one else can break good-morning.

  • urllib2 gone and replaced with @petercerno original code
  • updated readme; added example of financials class
  • created a tests folder with 3 simple unit tests; make sure a break like this doesn't happen again
  • Includes exception handling (unit test included)
    • exception for someone entering an empty string
    • exception for someone entering an incorrect ticket symbol
  • Include continuous integration to run unittests anytime some requests a pull requests.
    • Requires action from @petercerno , but will limit time spent reviewing pull requests
    • Action: Just add Travis CI and Appveyor to your github.

To add CI to good-morning it only takes a few clicks:

  1. Follow instructions here: https://docs.travis-ci.com/user/getting-started/#To-get-started-with-Travis-CI
  2. Follow instructions here: https://www.appveyor.com/docs/#step-1---sign-in-with-appveyor

Travis gives coverage for Linux and OSX; Appveyor gives coverage for Windows.

Now, when anyone does a pull request, the unit tests launch automatically, and if it doesn't pass, you know the pull request breaks good-morning.

linwoodc3 added 22 commits July 23, 2017 10:28
… and exception handling for passing empty string and invalid ticker symbol
…ling, adds unittests, and includeds contin. integration tests for Windows, Linux, OSX.
commit c1fcdae
Merge: fde97ac fd73447
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 14:08:59 2017 -0400

    Merge branch 'urlFix_linwoodError' of https://github.com/linwoodc3/good-morning into urlFix_linwoodError

commit fde97ac
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 10:28:18 2017 -0400

    Fixes petercerno#6 and petercerno#7 url problems, adds exception handling, adds unittests, and includeds contin. integration tests for Windows, Linux, OSX.

commit fd73447
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 13:36:25 2017 -0400

    Fixing the travis ci build. # 17

commit f58f591
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 13:17:00 2017 -0400

    Fixing the travis ci build. # 16

commit fcc539a
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 13:06:44 2017 -0400

    Fixing the travis ci build. # 15

commit 6fc4e9f
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 12:55:46 2017 -0400

    Fixing the travis ci build. # 14

commit 6d04c78
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 12:40:49 2017 -0400

    Fixing the travis ci build. # 13

commit a280acf
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 12:20:20 2017 -0400

    Fixing the travis ci build. # 12

commit 57abfd1
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 12:12:17 2017 -0400

    Fixing the travis ci build. # 11

commit 536ec5d
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 11:51:13 2017 -0400

    Fixing the travis ci build. # 9

commit 7e8a6d7
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 11:39:51 2017 -0400

    Fixing the travis ci build. # 8

commit 7e95db7
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 11:33:16 2017 -0400

    Fixing the travis ci build. # 7

commit d97535a
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 11:23:15 2017 -0400

    Fixing the travis ci build. # 6

commit 7b48720
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 11:19:08 2017 -0400

    Fixing the travis ci build. # 5

commit 64f6f12
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 11:09:12 2017 -0400

    Fixing the travis ci build. # 4

commit c8501e9
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 10:56:42 2017 -0400

    Fixing the travis ci build. # 3

commit 7853d38
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 10:50:37 2017 -0400

    Fixing the travis ci build. # 2

commit 9a00fbf
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 10:47:04 2017 -0400

    Fixing the travis ci build.

commit 3df0634
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 10:28:18 2017 -0400

    Fixes petercerno#6 and petercerno#7 url problems.  Also adds unit test and exception handling for passing empty string and invalid ticker symbol
commit c1fcdae
Merge: fde97ac fd73447
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 14:08:59 2017 -0400

    Merge branch 'urlFix_linwoodError' of https://github.com/linwoodc3/good-morning into urlFix_linwoodError

commit fde97ac
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 10:28:18 2017 -0400

    Fixes petercerno#6 and petercerno#7 url problems, adds exception handling, adds unittests, and includeds contin. integration tests for Windows, Linux, OSX.

commit fd73447
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 13:36:25 2017 -0400

    Fixing the travis ci build. # 17

commit f58f591
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 13:17:00 2017 -0400

    Fixing the travis ci build. # 16

commit fcc539a
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 13:06:44 2017 -0400

    Fixing the travis ci build. # 15

commit 6fc4e9f
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 12:55:46 2017 -0400

    Fixing the travis ci build. # 14

commit 6d04c78
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 12:40:49 2017 -0400

    Fixing the travis ci build. # 13

commit a280acf
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 12:20:20 2017 -0400

    Fixing the travis ci build. # 12

commit 57abfd1
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 12:12:17 2017 -0400

    Fixing the travis ci build. # 11

commit 536ec5d
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 11:51:13 2017 -0400

    Fixing the travis ci build. # 9

commit 7e8a6d7
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 11:39:51 2017 -0400

    Fixing the travis ci build. # 8

commit 7e95db7
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 11:33:16 2017 -0400

    Fixing the travis ci build. # 7

commit d97535a
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 11:23:15 2017 -0400

    Fixing the travis ci build. # 6

commit 7b48720
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 11:19:08 2017 -0400

    Fixing the travis ci build. # 5

commit 64f6f12
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 11:09:12 2017 -0400

    Fixing the travis ci build. # 4

commit c8501e9
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 10:56:42 2017 -0400

    Fixing the travis ci build. # 3

commit 7853d38
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 10:50:37 2017 -0400

    Fixing the travis ci build. # 2

commit 9a00fbf
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 10:47:04 2017 -0400

    Fixing the travis ci build.

commit 3df0634
Author: Linwood Creekmore III <[email protected]>
Date:   Sun Jul 23 10:28:18 2017 -0400

    Fixes petercerno#6 and petercerno#7 url problems.  Also adds unit test and exception handling for passing empty string and invalid ticker symbol
@linwoodc3
Copy link
Contributor Author

Closes #6. Closes #7

@petercerno
Copy link
Owner

I really appreciate your contribution and your enthusiasm for the project. Unfortunately, I cannot accept your pull request in the current form (as it is a bundle of two independent changes). Would it be possible to create another pull request containing only the bug fix (and nothing else)?

I do not want to add the CI code as it adds unnecessary complexity, i.e. I prefer to keep the code as simple as possible (without any distractions), and the library as small as possible.

@petercerno petercerno closed this Jul 24, 2017
@linwoodc3
Copy link
Contributor Author

No unittests either? That will check to see if the url return has changed or not.

@linwoodc3
Copy link
Contributor Author

And being transparent, the enthusiasm is around the utility of the code. I can use this library for "Buffett-style" security analysis, so, it helps me. :-)

@petercerno
Copy link
Owner

Ah, sorry, yes it is perfectly fine to add unit tests. Maybe add a small comment about how to run the unit tests (either in README, or the unit tests themselves). Thanks a lot!

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.

2 participants