Wouldn't it just make sense to remove asserts and use IF condition so that
we don't need to set any rules when to assert and when to not assert.
I don't see a huge benefit of using assert but I can see how it can be
mis-used (used when it shouldn't not be used) sometimes.
Regards,
Kaxil
On Wed, Dec 4, 2019 at 1:17 PM Anton Zayniev <anton.zayn...@gmail.com>
wrote:
> Ok, my thoughts on assertions.I think carefully used assertions could be
> occasionally useful for internal checks only. Like you checking all good
> and raise *your* exception with meaningful message to end user. But in
> general looks like confusion harm would outweight readability benefit.
> Just a simple AssertionError would confuse average pythonist as developer
> expects (and official docs clearly states that) assertion usage in test
> environment only. There are alternatives that are as readable as
> assertion. `assert self.futures, NOT_STARTED_MESSAGE` -> `if not
> self.futures: raise Error(NOT_STARTED_MESSAGE)`
>
> On Wed, 4 Dec 2019 at 09:04, Jarek Potiuk <jarek.pot...@polidea.com>
> wrote:
>
> > Hey @Anton - I think it would be great if (even if you have examples in
> > Github) add some summary of your thoughts here. I think we should
> converge
> > to one place where we discuss it and it's not helpful to move back/forth
> > between different media.
> >
> > The discussion gets interesting :).
> >
> > One argument I have for using asserts is to indeed treat them as
> > "debugging-purpose only" - it's actually pretty much the same as type
> > annotations. Type annotations are stripped out always when the code is
> run
> > so we cannot rely on them, yet they are super helpful in catching
> problems
> > and MyPy is really good in catching some mis-uses of flexibility of
> python
> > typing. And we are already using it.
> >
> > I think one of the issues (also mentioned by XD) is the "blur boundary".
> > Indeed we have some asserts now (some introduced by me :)) that are not
> > good for asserts but I think it's one more reason to agree some common
> > ground here and (if we decide that sometimes assertions are helpful and
> > recommended) to have clear rules describing where to use them.
> >
> > *Good examples:*
> >
> > This is a very good example where asserts are much clearer than if/throw
> in
> > my opinion. It's a programming error if end () is run without start().
> And
> > if asserts are stripped-out we got the "None" exceptions two lines down.
> >
> > def end(self) -> None:
> > """
> > Ends the executor.
> > :return:
> > """
> > assert self.impl, NOT_STARTED_MESSAGE
> > assert self.manager, NOT_STARTED_MESSAGE
> > self.impl.end()
> > self.manager.shutdown()
> >
> > Another good example of decorator for CLI methods (maybe the first assert
> > should have a meaningful message though).
> >
> > @functools.wraps(f)
> > def wrapper(*args, **kwargs):
> > """
> > An wrapper for cli functions. It assumes to have Namespace instance
> > at 1st positional argument
> >
> > :param args: Positional argument. It assumes to have Namespace
> instance
> > at 1st positional argument
> > :param kwargs: A passthrough keyword argument
> > """
> > assert args
> > assert isinstance(args[0], Namespace), \
> > "1st positional argument should be argparse.Namespace instance,
> " \
> > "but {}".format(args[0])
> >
> > Another good example: the project_id comes from the decorators and will
> > always be set to some value even if it is specified as Optional. If it is
> > not set in the environment, the object will never be instantiated in the
> > first place.
> >
> > @_fallback_to_project_id_from_variables
> > @GoogleCloudBaseHook.fallback_to_default_project_id
> > def is_job_dataflow_running(self, name: str, variables: Dict,
> > project_id: Optional[str] = None) -> bool:
> > """
> > Helper method to check if jos is still running in dataflow
> >
> > :param name: The name of the job.
> > :type name: str
> > :param variables: Variables passed to the job.
> > :type variables: dict
> > :param project_id: Optional, the GCP project ID in which to start a
> > job.
> > If set to None or missing, the default project_id from the GCP
> > connection is used.
> > :return: True if job is running.
> > :rtype: bool
> > """
> > assert project_id is not None
> >
> >
> > *Bad examples:*
> >
> > This depends on configuration so it should not be assert:
> >
> > if cluster_address is None:
> > cluster_address = conf.get('dask', 'cluster_address')
> > assert cluster_address, 'Please provide a Dask cluster address in
> > airflow.cfg'
> >
> > This also depends on configuration and should be an exception::
> >
> > executor_path = executor_name.split('.')
> > assert len(executor_path) == 2, f"Executor {executor_name} not supported:
> > " \
> > f"please specify in format
> > plugin_module.executor"
> >
> > J.
> >
> >
> > On Wed, Dec 4, 2019 at 2:37 AM Deng Xiaodong <xd.den...@gmail.com>
> wrote:
> >
> > > -1 from me for using asserts in Airflow code.
> > >
> > > Firstly, as some folks pointed out, asserts are there mainly for
> > debugging
> > > purposes.
> > >
> > > Second, even though using asserts may bring some benefits in specific
> > > context, it’s not enough to “break even” comparing with the potential
> > > confusions (e.g. blur boundary between when to use and when NOT to use
> > > asserts over raising exceptions).
> > >
> > >
> > > XD
> > >
> > > On Wed, Dec 4, 2019 at 06:17 Anton Zayniev <anton.zayn...@gmail.com>
> > > wrote:
> > >
> > > > Email is not so good for code examples, so I've shared my thoughts
> here
> > > > <https://github.com/apache/airflow/pull/6596/files#r353451761>.
> > > >
> > > > On Tue, 3 Dec 2019 at 16:53, Jarek Potiuk <jarek.pot...@polidea.com>
> > > > wrote:
> > > >
> > > > > Just an example of such asserts which IMHO are nicer are here:
> > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/apache/airflow/pull/6596/files#diff-4c0c36f193f2cd65e2b55ba3102c1ba2R38
> > > > > One line assert with message.
> > > > >
> > > > > J.
> > > > >
> > > > >
> > > > >
> > > > > On Tue, Dec 3, 2019 at 5:36 PM Anton Zayniev <
> > anton.zayn...@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Hi, guys. I'm really surprised about this
> > > > > >
> > > > > > > - (+) asserts look nicer and are more readable than if
> > (something):
> > > > > > > throw Exception()
> > > > > >
> > > > > > I'm pretty sure that all the code I have encountered a way more
> > > > readable
> > > > > > using "if/else" or "try/except". But may be it is just me. Could
> > you
> > > > > > provide an example of code which is better with "assert"?
> > > > > >
> > > > > > - (+) asserts are especially good for cases like None exception
> -
> > > they
> > > > > > > add more developer friendly messages when they will fail a
> few
> > > > lines
> > > > > > > below
> > > > > > > with (for example) None has no property "dag". But it's ok
> if
> > > > those
> > > > > > get
> > > > > > > optimised away.
> > > > > >
> > > > > > I think the best way to catch None is to ensure your code would
> > fail
> > > > > > conveniently. Like raising understandable Exception message, if
> you
> > > > > believe
> > > > > > that should be a point of confusion.
> > > > > >
> > > > > > On Tue, 3 Dec 2019 at 16:22, Iuliia Volkova <xnuins...@gmail.com
> >
> > > > wrote:
> > > > > >
> > > > > > > Hi everyone, I'm usually not write anything in this mail list,
> > but
> > > > this
> > > > > > > theme something really strange
> > > > > > > Exist offissial doc:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement
> > > > > > >
> > > > > > > and there is a key information: Assert statements are a
> > convenient
> > > > way
> > > > > to
> > > > > > > insert debugging assertions into a program.
> > > > > > >
> > > > > > > *Debugging. * - this is a key propose of asserts keyword.
> > > > > > >
> > > > > > > there is no any type of possible asserts that cannot be done
> with
> > > > > normal
> > > > > > > Exceptions and Errors types that more explicit and detailed
> when
> > > > > > 'assert' -
> > > > > > > you have ValueError, TyperError and etc. what kind of problems
> > must
> > > > > > solved
> > > > > > > DEBUG tools in production code that can be easily turned off on
> > > > servers
> > > > > > by
> > > > > > > users?
> > > > > > >
> > > > > > > asserts used in tests and in process of debug code, not in
> > > production
> > > > > > >
> > > > > > > On Tue, Dec 3, 2019 at 6:47 PM Jarek Potiuk <
> > > > jarek.pot...@polidea.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > We had a few discussions about using asserts in our code. I
> > > pasted
> > > > > some
> > > > > > > > links below but wanted to extract a gist of it.
> > > > > > > >
> > > > > > > > Here are the comments summarised:
> > > > > > > >
> > > > > > > > - (+) asserts look nicer and are more readable than if
> > > > > (something):
> > > > > > > > throw Exception()
> > > > > > > > - (-) asserts can be optimized away with -O flag so we
> > should
> > > > not
> > > > > > > based
> > > > > > > > any real logic on having them
> > > > > > > > - (+) asserts are good in cases that can happen in
> > development
> > > > but
> > > > > > > > should "never happen" in reality
> > > > > > > > - (+) asserts are especially good for cases like None
> > > exception
> > > > -
> > > > > > they
> > > > > > > > add more developer friendly messages when they will fail a
> > few
> > > > > lines
> > > > > > > > below
> > > > > > > > with (for example) None has no property "dag". But it's ok
> > if
> > > > > those
> > > > > > > get
> > > > > > > > optimised away.
> > > > > > > >
> > > > > > > > We would like to discuss those points in community and have a
> > > > > > community -
> > > > > > > > driven decision on:
> > > > > > > >
> > > > > > > > 1) whether we should use asserts?
> > > > > > > > 2) in which cases
> > > > > > > > 3) in which cases we should NOT use asserts.
> > > > > > > >
> > > > > > > > J.
> > > > > > > >
> > > > > > > > The links here:
> > > > > > > >
> > > > > > > > Slack Discussion:
> > > > > > > >
> > > > >
> > https://apache-airflow.slack.com/archives/CCQ7EGB1P/p1575364664041300
> > > > > > > >
> > > > > > > > Github threads:
> > > > > > > >
> > > > > > > > -
> > > > > https://github.com/apache/airflow/pull/6596#discussion_r352916409
> > > > > > > > -
> > > > > https://github.com/apache/airflow/pull/6596#discussion_r352914727
> > > > > > > > -
> > > > > > > >
> > > > > >
> > > >
> > https://github.com/apache/airflow/pull/3690#pullrequestreview-143376629
> > > > > > > >
> > > > > > > > Stack overflow link for asserts:
> > > > > > > >
> > > > > > > > - https://stackoverflow.com/a/1838411/5691525
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > J.
> > > > > > > >
> > > > > > > > --
> > > > > > > >
> > > > > > > > Jarek Potiuk
> > > > > > > > Polidea <https://www.polidea.com/> | Principal Software
> > Engineer
> > > > > > > >
> > > > > > > > M: +48 660 796 129 <+48660796129>
> > > > > > > > [image: Polidea] <https://www.polidea.com/>
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > _________
> > > > > > >
> > > > > > > С уважением, Юлия Волкова
> > > > > > > Тел. : +7 (911) 116-71-82
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > >
> > > > > Jarek Potiuk
> > > > > Polidea <https://www.polidea.com/> | Principal Software Engineer
> > > > >
> > > > > M: +48 660 796 129 <+48660796129>
> > > > > [image: Polidea] <https://www.polidea.com/>
> > > > >
> > > >
> > >
> >
> >
> > --
> >
> > Jarek Potiuk
> > Polidea <https://www.polidea.com/> | Principal Software Engineer
> >
> > M: +48 660 796 129 <+48660796129>
> > [image: Polidea] <https://www.polidea.com/>
> >
>
