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

Rework whole Package to work under Python 2+3 and tests #206

Closed
wants to merge 34 commits into
base: master
from

Conversation

@loechel
Member

loechel commented Jul 6, 2017

Rework the whole Package to make it testable and follow best practices.
Used tox and pytest as new test environment.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 6, 2017

Coverage Status

Coverage increased (+28.8%) to 83.75% when pulling 83724b8 on tox into f5c2759 on master.

coveralls commented Jul 6, 2017

Coverage Status

Coverage increased (+28.8%) to 83.75% when pulling 83724b8 on tox into f5c2759 on master.

loechel referenced this pull request in plone/plone_best_practices_discussion Jul 12, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 15, 2017

Coverage Status

Coverage increased (+43.1%) to 98.125% when pulling 71e8b1c on tox into f5c2759 on master.

coveralls commented Jul 15, 2017

Coverage Status

Coverage increased (+43.1%) to 98.125% when pulling 71e8b1c on tox into f5c2759 on master.

@loechel

This comment has been minimized.

Show comment
Hide comment
@loechel

loechel Jul 15, 2017

Member

@MrTango / @gforcada ready for you.

Member

loechel commented Jul 15, 2017

@MrTango / @gforcada ready for you.

Show outdated Hide outdated tests/test_hooks.py
with pytest.raises(AttributeError):
hooks.validate_packagename(None)
configurator = Configurator(

This comment has been minimized.

@MrTango

MrTango Jul 18, 2017

Contributor

Is here an indention missing?

@MrTango

MrTango Jul 18, 2017

Contributor

Is here an indention missing?

This comment has been minimized.

@loechel

loechel Jul 18, 2017

Member

no. that is like it should.

I do test various variants within one test method.
As hooks.validate_packagename() should take a Configurator object but also tests against None did have for all of those case test steps. Maybe it would be good to put comments for each case.

@loechel

loechel Jul 18, 2017

Member

no. that is like it should.

I do test various variants within one test method.
As hooks.validate_packagename() should take a Configurator object but also tests against None did have for all of those case test steps. Maybe it would be good to put comments for each case.

@loechel

This comment has been minimized.

Show comment
Hide comment
@loechel

loechel Jul 18, 2017

Member

@MrTango commented to make it clearer

Member

loechel commented Jul 18, 2017

@MrTango commented to make it clearer

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 18, 2017

Coverage Status

Coverage increased (+43.1%) to 98.125% when pulling d70ef01 on tox into f5c2759 on master.

coveralls commented Jul 18, 2017

Coverage Status

Coverage increased (+43.1%) to 98.125% when pulling d70ef01 on tox into f5c2759 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 18, 2017

Coverage Status

Coverage increased (+43.1%) to 98.125% when pulling d70ef01 on tox into f5c2759 on master.

coveralls commented Jul 18, 2017

Coverage Status

Coverage increased (+43.1%) to 98.125% when pulling d70ef01 on tox into f5c2759 on master.

@MrTango

This comment has been minimized.

Show comment
Hide comment
@MrTango

MrTango Aug 3, 2017

Contributor

@loechel can you please add some docs. I also can not run the buildout, it crashes:

    return installer.install(specs, working_set)
  File "/home/maik/develop/plonecore/bobtemplates.plone/local/lib/python2.7/site-packages/zc/buildout/easy_install.py", line 708, in install
    extra_requirements = dist.requires(req.extras)[::-1]
  File "/home/maik/develop/plonecore/bobtemplates.plone/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2559, in requires
    "%s has no such extra feature %r" % (self, ext)
UnknownExtra: bobtemplates.plone 1.0.6.dev0 has no such extra feature 'test'

I also added a requirements.txt.

Contributor

MrTango commented Aug 3, 2017

@loechel can you please add some docs. I also can not run the buildout, it crashes:

    return installer.install(specs, working_set)
  File "/home/maik/develop/plonecore/bobtemplates.plone/local/lib/python2.7/site-packages/zc/buildout/easy_install.py", line 708, in install
    extra_requirements = dist.requires(req.extras)[::-1]
  File "/home/maik/develop/plonecore/bobtemplates.plone/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2559, in requires
    "%s has no such extra feature %r" % (self, ext)
UnknownExtra: bobtemplates.plone 1.0.6.dev0 has no such extra feature 'test'

I also added a requirements.txt.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 3, 2017

Coverage Status

Coverage increased (+43.1%) to 98.125% when pulling 27d3a05 on tox into f5c2759 on master.

coveralls commented Aug 3, 2017

Coverage Status

Coverage increased (+43.1%) to 98.125% when pulling 27d3a05 on tox into f5c2759 on master.

@loechel

This comment has been minimized.

Show comment
Hide comment
@loechel

loechel Aug 3, 2017

Member

@MrTango I added some documentation and removed more unnecessary files like buildout.cfg and requirements.txt

You do not need any buildout on this package anymore it is fully pip compatible and should only be developed like that and also tested.

Member

loechel commented Aug 3, 2017

@MrTango I added some documentation and removed more unnecessary files like buildout.cfg and requirements.txt

You do not need any buildout on this package anymore it is fully pip compatible and should only be developed like that and also tested.

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Aug 3, 2017

Member

If it is pip compatible I would expect a requirements.txt file...

Member

jensens commented Aug 3, 2017

If it is pip compatible I would expect a requirements.txt file...

@loechel

This comment has been minimized.

Show comment
Hide comment
@loechel

loechel Aug 3, 2017

Member

@jensens why,

A good python package should define all of it requirements in the setup.py even the constrains. requirements.txt and constains.txt are for python projects, where you plug several packages together.

So there is no need for a requirements.txt or buildout.cfg or anything project related here as it is a plugin for mr.bob and not a project itself.

Member

loechel commented Aug 3, 2017

@jensens why,

A good python package should define all of it requirements in the setup.py even the constrains. requirements.txt and constains.txt are for python projects, where you plug several packages together.

So there is no need for a requirements.txt or buildout.cfg or anything project related here as it is a plugin for mr.bob and not a project itself.

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Aug 4, 2017

Member

@loechel it is more about version pins, which should not happen (except minimum versions) in setup.[py|cfg]. So I agree not adding requirements.txt, but if we don't use buildout, constraints.txt is a great place for this

Member

jensens commented Aug 4, 2017

@loechel it is more about version pins, which should not happen (except minimum versions) in setup.[py|cfg]. So I agree not adding requirements.txt, but if we don't use buildout, constraints.txt is a great place for this

@loechel

This comment has been minimized.

Show comment
Hide comment
@loechel

loechel Aug 10, 2017

Member

Ok I have now moved the test matrix into the travis-ci config again.
It tests now all. but the fattheme test itself fail, could someone else please have a look.

Member

loechel commented Aug 10, 2017

Ok I have now moved the test matrix into the travis-ci config again.
It tests now all. but the fattheme test itself fail, could someone else please have a look.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 10, 2017

Coverage Status

Changes Unknown when pulling 133495b on tox into ** on master**.

coveralls commented Aug 10, 2017

Coverage Status

Changes Unknown when pulling 133495b on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 10, 2017

Coverage Status

Changes Unknown when pulling 5708c60 on tox into ** on master**.

coveralls commented Aug 10, 2017

Coverage Status

Changes Unknown when pulling 5708c60 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 10, 2017

Coverage Status

Changes Unknown when pulling 5708c60 on tox into ** on master**.

coveralls commented Aug 10, 2017

Coverage Status

Changes Unknown when pulling 5708c60 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 10, 2017

Coverage Status

Changes Unknown when pulling 5708c60 on tox into ** on master**.

coveralls commented Aug 10, 2017

Coverage Status

Changes Unknown when pulling 5708c60 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 10, 2017

Coverage Status

Changes Unknown when pulling 5708c60 on tox into ** on master**.

coveralls commented Aug 10, 2017

Coverage Status

Changes Unknown when pulling 5708c60 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 10, 2017

Coverage Status

Changes Unknown when pulling 04e5ac7 on tox into ** on master**.

coveralls commented Aug 10, 2017

Coverage Status

Changes Unknown when pulling 04e5ac7 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 10, 2017

Coverage Status

Changes Unknown when pulling 04e5ac7 on tox into ** on master**.

coveralls commented Aug 10, 2017

Coverage Status

Changes Unknown when pulling 04e5ac7 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 10, 2017

Coverage Status

Changes Unknown when pulling 04e5ac7 on tox into ** on master**.

coveralls commented Aug 10, 2017

Coverage Status

Changes Unknown when pulling 04e5ac7 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 10, 2017

Coverage Status

Changes Unknown when pulling 04e5ac7 on tox into ** on master**.

coveralls commented Aug 10, 2017

Coverage Status

Changes Unknown when pulling 04e5ac7 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 10, 2017

Coverage Status

Changes Unknown when pulling 04e5ac7 on tox into ** on master**.

coveralls commented Aug 10, 2017

Coverage Status

Changes Unknown when pulling 04e5ac7 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 10, 2017

Coverage Status

Changes Unknown when pulling 04e5ac7 on tox into ** on master**.

coveralls commented Aug 10, 2017

Coverage Status

Changes Unknown when pulling 04e5ac7 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 11, 2017

Coverage Status

Changes Unknown when pulling 4b6ace6 on tox into ** on master**.

coveralls commented Aug 11, 2017

Coverage Status

Changes Unknown when pulling 4b6ace6 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 11, 2017

Coverage Status

Changes Unknown when pulling 4b6ace6 on tox into ** on master**.

coveralls commented Aug 11, 2017

Coverage Status

Changes Unknown when pulling 4b6ace6 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 11, 2017

Coverage Status

Changes Unknown when pulling 4b6ace6 on tox into ** on master**.

coveralls commented Aug 11, 2017

Coverage Status

Changes Unknown when pulling 4b6ace6 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 11, 2017

Coverage Status

Changes Unknown when pulling 4b6ace6 on tox into ** on master**.

coveralls commented Aug 11, 2017

Coverage Status

Changes Unknown when pulling 4b6ace6 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 11, 2017

Coverage Status

Changes Unknown when pulling 4b6ace6 on tox into ** on master**.

coveralls commented Aug 11, 2017

Coverage Status

Changes Unknown when pulling 4b6ace6 on tox into ** on master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 11, 2017

Coverage Status

Changes Unknown when pulling 4b6ace6 on tox into ** on master**.

coveralls commented Aug 11, 2017

Coverage Status

Changes Unknown when pulling 4b6ace6 on tox into ** on master**.

@loechel

This comment has been minimized.

Show comment
Hide comment
@loechel

loechel Sep 14, 2017

Member

this pull gets partial superseeded by #216

Member

loechel commented Sep 14, 2017

this pull gets partial superseeded by #216

@loechel

This comment has been minimized.

Show comment
Hide comment
@loechel

loechel Oct 3, 2017

Member

most of the stuff from this branch has already been merged into 3.0. so I close this pull request.

Member

loechel commented Oct 3, 2017

most of the stuff from this branch has already been merged into 3.0. so I close this pull request.

@loechel loechel closed this Oct 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment