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/>
>
