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

ENH+BF: fix up of some tests/docs to pass with python2.7, testing on travis, consistent shebanging, #144

Merged
merged 22 commits into from
May 21, 2016

Conversation

yarikoptic
Copy link
Contributor

I really loved the idea and the collection so decided to contribute a bit ;)

https://travis-ci.org/yarikoptic/python-patterns/builds/130877280
should eventually reflect current state of things -- either tests pass as is or more things to be done

Please enable travis for your project @faif -- it is easy, just go to http://travis-ci.org, authenticate through github, click toggle for this repository

I have already complained in #143 about necessity to have clear copyright/license statement(s) so this code could e.g. be actually used/redistributed etc. Ideally the most permissive license (BSD-2 or -3 or MIT) would be the best fit.

Also would be nice to make all code PEP8 compliant. I have added call to flake8 in travis.yml but ignoring its failure atm. Didn't want to introduce big "stylistic" changes without "go ahead"

@yarikoptic
Copy link
Contributor Author

ok -- with a small tune up (could not do || : for some reason), now all splendid: https://travis-ci.org/yarikoptic/python-patterns/builds/130880031

also here is coverage report from https://codecov.io/gh/yarikoptic/python-patterns (also enable for your repo, and then can add a badge or two to your README etc)

@fkromer
Copy link
Contributor

fkromer commented May 17, 2016

I did not get why lines 81, 61 and 91 in https://codecov.io/gh/yarikoptic/python-patterns/src/ef3dbe3b17b10f9926607f98fa10b1913c61476f/test_hsm.py are not covered...

@yarikoptic
Copy link
Contributor Author

Pushed more fixes and trying to enhance testing by actually running all of the scripts. It required also fixing 3-tier.py by reverting 69b507b -- descriptors must be bound to classes, not to instances, thus that change broke the "pythonish" idea completely. Could be redone without relying on Python descriptors at the instance level

@yarikoptic
Copy link
Contributor Author

woohoo -- tests pass again (although I have dropped pypy* for now, was getting stuck), and coverage is whooping 93%: see https://codecov.io/gh/yarikoptic/python-patterns on what to improve. You can easily get 100% with little effort!

@yarikoptic
Copy link
Contributor Author

On Tue, 17 May 2016, thinwybk wrote:

I did not get why lines 81, 61 and 91 in
https://codecov.io/gh/yarikoptic/python-patterns/src/ef3dbe3b17b10f9926607f98fa10b1913c61476f/test_hsm.py
are not covered...

ain't auto coverage great?! ;)

my guess, is that since it is assertRaises, is that previous lines cause
the exception, so it leaves the context manager scope before executing
them. Fix should be easy (if legit) to just dedent those lines out of
the context manager scope ;)

Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik

@yarikoptic
Copy link
Contributor Author

On Tue, 17 May 2016, Yaroslav Halchenko wrote:

On Tue, 17 May 2016, thinwybk wrote:

I did not get why lines 81, 61 and 91 in
https://codecov.io/gh/yarikoptic/python-patterns/src/ef3dbe3b17b10f9926607f98fa10b1913c61476f/test_hsm.py
are not covered...

ain't auto coverage great?! ;)

my guess, is that since it is assertRaises, is that previous lines cause
the exception, so it leaves the context manager scope before executing
them. Fix should be easy (if legit) to just dedent those lines out of
the context manager scope ;)

ok -- did that for that file... there might be more...
also added some pragma no cover's for points which it is ok to expect no
coverage

Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik

@yarikoptic
Copy link
Contributor Author

ok -- test_hsm is now at 100% ;) overall: codecov

@fkromer
Copy link
Contributor

fkromer commented May 17, 2016

Thanks a lot :)

@fkromer
Copy link
Contributor

fkromer commented May 17, 2016

Could you merge https://github.com/fkromer/python-patterns/tree/test_proxy (#139) into your fork to make sure these tests are still running ;)

* pr-139:
  change: refactor (work -> talk)
  change: refactoring self->cls
@yarikoptic
Copy link
Contributor Author

On Tue, 17 May 2016, thinwybk wrote:

Could you merge https://github.com/fkromer/python-patterns/tree/test_proxy
(#139) into your fork to make sure these tests are still running ;)

heh -- only for you and only today... merged, fixed it up the
test although don't like what I have done and the test logic/duplication
in general. but better some test than no tests! ;)

Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik

…version specific import of StringIO (breaks on 2.x with io.StringIO)
@fkromer
Copy link
Contributor

fkromer commented May 17, 2016

Thanks! That's just how it is (right now). :)

@faif
Copy link
Owner

faif commented May 18, 2016

Hi,

Thanks for your contributions. Sorry for not taking any actions yet but I'm ill. I will check everything after recovering.

Kind regards,
Sakis

On 17 Μαΐ 2016, at 23:33, thinwybk [email protected] wrote:

Thanks! That's just how it is (right now). :)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@yarikoptic
Copy link
Contributor Author

Thanks for your contributions. Sorry for not taking any actions yet but
I'm ill. I will check everything after recovering.

sure -- no rush. Get better!

@faif faif merged commit b54cad9 into faif:master May 21, 2016
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.

3 participants