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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Makes sense, just a few questions inline. Thank you :)



================
Comment at: llvm/utils/TableGen/jupyter/LLVM_TableGen.md:120
+```tablegen
+%noreset
+// This works because of the noreset above.
----------------
I guess that one more example could be:
```
%reset
%noreset
```


================
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"])
----------------
Comments like this are super helpful, but I guess that you will need to review them every time this method is updated?


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_kernel/kernel.py:143
+        name, value = config
+        # Curently cache reset is the only possible setting.
+        if name != "cellreset":
----------------
[nit] I would skip this. The code is self-explanatory and this comment is quite likely to get out of date soon.


================
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
+
----------------
Are there any benefits to allowing `%reset` and `%noreset` in one cell? IMO, it could lead to some counter-intuitive behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149055