[PATCH] D149055: [llvm][TableGen][Jupyter] Add configurable default reset behaviour

[David Spickett via Phabricator via llvm-commits]

DavidSpickett marked an inline comment as done.
DavidSpickett added inline comments.


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_kernel/kernel.py:112-135
+        >>> k.parse_config_magic([])
+        Traceback (most recent call last):
+         ...
+        TableGenKernelException: Incorrect number of parameters to %config. Expected %config <setting> <value>.
+        >>> k._cell_reset
+        False
+        >>> k.parse_config_magic(["a", "b", "c"])
----------------
awarzynski wrote:
> Comments like this are super helpful, but I guess that you will need to review them every time this method is updated?
These are doctests https://docs.python.org/3/library/doctest.html. So yes, someone has to remember to run them, but that person was presumably editing this method so it's not difficult to notice them.


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_kernel/kernel.py:230-231
+        # Magic reset commands always win if present.
+        reset = magic.get("reset") is not None
+        noreset = magic.get("noreset") is not None
+
----------------
awarzynski wrote:
> Are there any benefits to allowing `%reset` and `%noreset` in one cell? IMO, it could lead to some counter-intuitive behavior.
```
%reset
%noreset
```
Should follow the last one wins precedent set by `%args` but as implemented it wouldn't. Also I'm not sure you'd ever intentionally do this, good reason to not allow it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149055/new/

https://reviews.llvm.org/D149055