Closed Bug 647244 Opened 13 years ago Closed 13 years ago

remove assert from code

Categories

(Cloud Services :: Server: Share, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: tarek, Assigned: rmiller)

References

Details

(Whiteboard: [0.5d])

plain assert in production code is a bad practice:

- the callers that might want to catch the raise need something less fuzzy
- the call that does the assertion need a better exception strategy, like defining custom exception for each case (ConfigurationError, etc..)
- assert gets *removed* if python is called with the --optimize flag
Assignee: nobody → rmiller
I think this is a generalization that isn't always correct.  I agree assertions should never be used to catch things like invalid configuration or invalid params to "public" methods/entry-points or anything that "could possibly happen" - but asserts can be useful to trap things that can "never happen", in which case none of the points above apply:

* Callers never want to catch it as it can't happen.
* No better exception strategy is needed as it can't happen.
* Having the assert removed with --optimize is fine - it can't happen

:)  The value in such asserts is primarily documentation for future maintainers and early diagnostic of invariants being changed during development.  They can also help achieve 100% branch coverage - eg:

  assert some_condition # we've already checked that!

versus:

  if not some_condition:
    raise SomeObscureException()

Where you can't reasonable get coverage of that branch as it can't happen.  The third alternative of removing it completely doesn't seem like much of a "win" to me - the assertion holds value to someone reading/changing the code for the first time.

In summary: While asserts can be abused and misused, they also have value when used appropriately.
Can you point out where we would want to keep those asserts in F1 and not turn them into real production errors or simple comments ?

I still don't see the value to have this else where than in unit/functional tests. Keeping them in code implies that you expect them to be called at some point, so they are part of the code and can impact the behavior.

For example, I had to remove a bunch of asserts from Distutils in Python because the package was behaving differently using -O

So if those assert are changing the server behavior, while they were supposed to be only documentation for developers as you said, that's a problem.
(In reply to comment #2)
> Can you point out where we would want to keep those asserts in F1 and not turn
> them into real production errors or simple comments ?

Given my comments above, there should be zero asserts which can not be removed and replaced with comments.  IMO though, asserts have higher value than comments - they are like "provable comments"

It almost sounds like you are saying asserts should not be in the language at all?  Or to turn it around, when do you think assert statements are reasonable to use, and assuming there are such cases, why would they not apply to f1?
(In reply to comment #3)
> (In reply to comment #2)
> > Can you point out where we would want to keep those asserts in F1 and not turn
> > them into real production errors or simple comments ?
> 
> Given my comments above, there should be zero asserts which can not be removed
> and replaced with comments.  IMO though, asserts have higher value than
> comments - they are like "provable comments"
>
> It almost sounds like you are saying asserts should not be in the language at
> all?  Or to turn it around, when do you think assert statements are reasonable
> to use, and assuming there are such cases, why would they not apply to f1?

They are fine and important to use in tests. In fact most people that use nose are using plain asserts in their tests.

Other than that... 

But I have grepped the stdlib and found many of them, changing the behavior of the code. So I'll drop a mail at python-dev to ask for their removal, and we'll see other opinions there, that could be interesting !
(In reply to comment #4)
> In fact most people that use nose
> are using plain asserts in their tests.

Surely that in itself is an anti-pattern - it can be very useful to run the test suite with -O, right?  IIUC, that is why unittest itself tends to explicitly raise AssertionError rather than relying on assert.

> So I'll drop a mail at python-dev to ask for their removal, and we'll
> see other opinions there, that could be interesting !

It will indeed :)
(In reply to comment #5)
> (In reply to comment #4)
> > In fact most people that use nose
> > are using plain asserts in their tests.
> 
> Surely that in itself is an anti-pattern - it can be very useful to run the
> test suite with -O, right? 
> IIUC, that is why unittest itself tends to
> explicitly raise AssertionError rather than relying on assert.

Mmm, that's very true. It seems that Nose is partially safe on this, as it just provides unittest methods, renamed: https://code.google.com/p/python-nose/source/browse/nose/tools.py#173

But those will be specificaly broken: https://code.google.com/p/python-nose/source/browse/nose/tools.py#22

> 
> > So I'll drop a mail at python-dev to ask for their removal, and we'll
> > see other opinions there, that could be interesting !
> 
> It will indeed :)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.