Conversation
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
|
🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 53a84b6 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F148831%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
ZeroIntensity
left a comment
There was a problem hiding this comment.
The C code and the corresponding documentation look good to me. I didn't look very closely at the tests.
|
Buildbot run: A few failures look unrelated But I also see https://buildbot.python.org/#/builders/1865/builds/54 Which is a bit mysterious, is this looking at the builtins of another build? |
Yes. We need to patch this test. It tests pickle compatiblity acrosses python versions. |
diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py
index 6366f12257..c2018c9785 100644
--- a/Lib/test/pickletester.py
+++ b/Lib/test/pickletester.py
@@ -3244,6 +3244,7 @@ def test_builtin_types(self):
'BuiltinImporter': (3, 3),
'str': (3, 4), # not interoperable with Python < 3.4
'frozendict': (3, 15),
+ 'sentinel': (3, 15),
}
for t in builtins.__dict__.values():
if isinstance(t, type) and not issubclass(t, BaseException): |
I opened #148967 to track it. |
| *name* and :attr:`~sentinel.__module__` set to *module_name*. | ||
| Return ``NULL`` with an exception set on failure. | ||
|
|
||
| *module_name* should be the name of an importable module if the sentinel |
There was a problem hiding this comment.
Please mention whether NULL is allowed for module_name to indicate that the sentinel doesn't support pickle.
There was a problem hiding this comment.
It is not. Should we support this?
There was a problem hiding this comment.
We might as well, since we already set the module to None if caller() doesn't find anything.
|
Thanks for the feedback! Pushed a new version. |
| class SentinelTest(unittest.TestCase): | ||
|
|
||
| def test_pysentinel_new(self): | ||
| import pickle |
There was a problem hiding this comment.
I think you can put this at the top-level. It's not really important that imports are fast in test modules.
|
|
||
| .. attribute:: __name__ | ||
|
|
||
| The sentinel's name. | ||
|
|
||
| .. attribute:: __module__ | ||
|
|
||
| The name of the module where the sentinel was created. |
There was a problem hiding this comment.
Maybe indicate something like "A sentinel object exposes the following read-only attributes" so that the paragraph about pickle is not glued to it.
| support.gc_collect() | ||
| self.assertIsNone(ref()) | ||
|
|
||
| def test_sentinel_union(self): |
There was a problem hiding this comment.
Maybe add a test where the RHS/LHS is not a type?
| with self.assertRaises(AttributeError): | ||
| missing.__module__ = "changed" |
There was a problem hiding this comment.
Maybe add tests to check that we can't delete those attributes either
| Sentinels importable from their defining module by name preserve their | ||
| identity when pickled and unpickled. Sentinels that are not importable by | ||
| module and name are not picklable. |
There was a problem hiding this comment.
How about move this paragraph down to the pickle part?
There was a problem hiding this comment.
Actually this is mostly redundant with the later pickle paragraph, I'll remove most of it.
Replaces JelleZijlstra#4. See python/steering-council#258.
📚 Documentation preview 📚: https://cpython-previews--148831.org.readthedocs.build/