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

Changed handling of decorators for classes in python 2.7 #9

Merged
merged 6 commits into from
Apr 19, 2018
Merged

Conversation

Chilipp
Copy link
Owner

@Chilipp Chilipp commented Apr 18, 2018

This should finally solve the troubles with python 2 mentioned in #5 and #2.

Pinging @lesteve and @guillaumeeb because of their previous interest.

@Chilipp
Copy link
Owner Author

Chilipp commented Apr 18, 2018

If you're happy with this solution, I would merge it and create a new release (0.2.3). I added some notes in the installation docs and the changelog about the new behavior.

@lesteve
Copy link
Contributor

lesteve commented Apr 19, 2018

Nice ! Glancing at the diff this looks like the simpler try/catch solution I had in mind (with additional bells and whistles for backward-compat).

Maybe I am missing something, but the snippet from #6 (comment) still raises an exception. Maybe you forgot to add the try/catch in with_indent?

@coveralls
Copy link

coveralls commented Apr 19, 2018

Coverage Status

Coverage increased (+1.2%) to 97.423% when pulling 0ba440b on dev into 14c2fd6 on master.

@Chilipp
Copy link
Owner Author

Chilipp commented Apr 19, 2018

Good points 😅 That's why I do not like copy-and-paste. This should be solved by the changes in 614796d

@lesteve
Copy link
Contributor

lesteve commented Apr 19, 2018

I tried and the snippet mentioned above runs fine, thanks!

@self.ds
class Test2(object):
"""docs"""
except AttributeError:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a side-comment (not saying that you should do it in this PR at all) but with pytest you can check expected exception like this:

with pytest.raises(AttributeError, 'the regex to check):
    # code that raises

See this for more details.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep I know. I usually force myself to use the unittest framework only and there is no such thing for python 2, only for python 3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually force myself to use the unittest framework only

Interesting, do you mind to elaborate why?

Copy link
Owner Author

@Chilipp Chilipp Apr 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I mainly base my tests on unittest because it integrates well with the object-oriented structure of my packages. Additionally, since it is a built-in library, it is very stable and keeps my dependencies for running the tests low. Anyway, of course I use pytest as well, especially since it provides a better interfaces for running the tests. But if it is only for a minor thing (like here, where I would only use it for this single test method) I'd rather not rely on non built-in libraries. But for other packages with more extensive test suites, I indeed often rely on pytest but still use unittest as a basis

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks for the details. I have to say that there are so many nice things included in pytest both for writing tests and running the tests that you would have to pry it from my cold dead hands ...

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆 agreed 😜

@Chilipp Chilipp merged commit 5154fcd into master Apr 19, 2018
@Chilipp Chilipp deleted the dev branch April 19, 2018 20:47
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.

None yet

3 participants