Python Metadata Validation #3502
Conversation
…ld system apparently.
cpp/src/slice2py/PythonUtil.cpp
Outdated
| @@ -2426,8 +2403,7 @@ Slice::Python::getImportFileName(const string& file, const UnitPtr& ut, const ve | |||
| void | |||
| Slice::Python::generate(const UnitPtr& un, bool all, const vector<string>& includePaths, Output& out) | |||
| { | |||
| Slice::Python::MetadataVisitor visitor; | |||
| un->visit(&visitor); | |||
| validateMetadata(un); | |||
There was a problem hiding this comment.
we should name the UnitPtr parameter unit.
There was a problem hiding this comment.
I fixed it in these locations, but in general we have many different (and inconsistent) names for unit.
unt is my personal favorite. We just have to save that i character...
cpp/src/slice2py/PythonUtil.cpp
Outdated
| bool | ||
| Slice::Python::MetadataVisitor::visitUnitStart(const UnitPtr& unit) | ||
| void | ||
| Slice::Python::validateMetadata(const UnitPtr& u) |
cpp/src/slice2py/PythonUtil.cpp
Outdated
| SequencePtr seq = dynamic_pointer_cast<Sequence>(type); | ||
| if (seq) | ||
| // If 'python:package' is applied to a module, it must be a top-level module. | ||
| // // Top-level modules are contained by the 'Unit'. Non-top-level modules are contained in 'Module's. |
There was a problem hiding this comment.
Good catch! Fixed.
| .validArgumentValues = {{"default", "list", "tuple"}}, | ||
| .acceptedContext = MetadataApplicationContext::DefinitionsAndTypeReferences, | ||
| }; | ||
| MetadataInfo unqualifiedSeqInfo = { |
There was a problem hiding this comment.
It would be clearer to define this variable after:
knownMetadata.emplace("python:seq", std::move(seqInfo));There was a problem hiding this comment.
Fine, I moved it.
Yes. I think this is correct.
Yes. We should have the same restrictions |
| knownMetadata.emplace("python:numpy.ndarray", std::move(arrayTypeInfo)); | ||
|
|
||
| // "python:memoryview" | ||
| MetadataInfo memoryViewInfo = { |
There was a problem hiding this comment.
Can we reuse the validation function from above? They all have the same restrictions.
There was a problem hiding this comment.
Yes we can; fixed.
cpp/src/slice2py/PythonUtil.cpp
Outdated
| if (seq) | ||
| // If 'python:package' is applied to a module, it must be a top-level module. | ||
| // // Top-level modules are contained by the 'Unit'. Non-top-level modules are contained in 'Module's. | ||
| if (auto mod = dynamic_pointer_cast<Module>(p); mod && !dynamic_pointer_cast<Unit>(mod->container())) |
There was a problem hiding this comment.
Maybe we should add a isTopLevel in modules, I think we have checks like this in a few places.
There was a problem hiding this comment.
I added this function to the parser, and used it everywhere where we could use it.
I've been thinking about adding this for a while, so I already had a list of places to use it!
One day I still need to add a getTopLevelModule function somewhere as well...
This PR switches
slice2pyto use the 'new' metadata validator (I added 3 months ago).The implementation, old validation, and docs disagree about these metadata, so insight would be appreciated:
python:array.array,python:numpy.ndarray, andpython:memoryview:XThe docs state that
array.arrayandnumpy.ndarrayare only valid on:integral builtin types excluding strings. But:bool,float, anddouble.So... I'm guessing that the documentation is just wrong. And it should say:
... are valid on sequences of numeric built-in types and boolsFor
python:memoryview:X.The docs list no restriction on what this can be applied to.
But, the old validation for
memoryviewre-used the logic forarrayandndarray, restricting what it can be applied to.Is this correct; that
memoryviewcan only go on sequences of built-in numeric types and bools?Or did we incorrectly re-use logic, and in reality, this metadata can be used with any sequence type?