fdemmer / django-weasyprint

A Django class-based view generating PDF resposes using WeasyPrint
Other
352 stars 53 forks source link

Added tests #22

Closed ZuluPro closed 5 years ago

ZuluPro commented 5 years ago

I added:

ZuluPro commented 5 years ago

@fdemmer Any news ?

fdemmer commented 5 years ago

first of all, thank you for contributing, your time and effort is appreciated! :+1:

i am not sure about the pull-request as is though. i am a "purist" in the sense, that i think "less code is more". every line added potentially adds bugs and needs to be maintained. sometimes it is good to take a step back and ask, what do i want, what do i need to focus on and then only do that.

the pull-request adds a whole test project only to have one test actually run and that test mostly verifies that django and the test-project itself works. it does not verify any of the expected response headers (and you left a set_trace() in there).

so, i am going to reject the pull-request.

however, i also think tests are important. they must focus on testing what the library does though and should not cover aspects out of its control.

i am thinking tests should:

thanks again for contributing!