Codegen: allow synth properties of non-synth classes
#13258
Conversation
redsun82
changed the title
synth properties of non-synth classessynth properties of non-synth classes
redsun82
force-pushed
the
redsun82/swift-synth-properties
branch
from
addae60 to
cc271d6
Compare
| } | ||
|
|
||
|
|
||
| def test_stub_on_class_with_ipa_on_arguments(generate_classes): | ||
| assert generate_classes([ | ||
| schema.Class("MyObject", ipa=schema.IpaInfo(on_arguments={"base": "A", "index": "int", "label": "string"})), | ||
| schema.Class("MyObject", ipa=schema.IpaInfo(on_arguments={"base": "A", "index": "int", "label": "string"}), | ||
| properties=[schema.SingleProperty("foo", "bar")]), |
There was a problem hiding this comment.
Am I reading this correctly that when you add a simple foo: bar property, it becomes synth=True by default? Or is it because this class is declared as inheriting from class "A" and its foo: bar | synth property on line 575 of test_dbschemegen.py? Basically, I'm finding it hard to follow how the fixtures for this test are set up.
There was a problem hiding this comment.
The idea is that all properties of synth classes are synth by default, without needing to explicitly mark it. It might be a bit confusing that synth actually translates to the ipa attribute internally (for historicaly reasons mainly). Would it be clearer if we wrote
assert generate_classes([
schema.Class("MyObject", synth=schema.SynthInfo(on_arguments={"base": "A", "index": "int", "label": "string"}),
properties=[schema.SingleProperty("foo", "bar")]),
]) ==(that is, substitute all ipa mentions with synth)?
Also, these test files are completely independent, and each tests each python module in isolation. So there's no need to look at test_dbschemegen.py for this test. The *gen.py tests are mostly built around testing that a certain input to the generation (typically a list of schema.Class instances) gives a certain output that is actually fed to the renderer. The tests themselves can be written quite simply, but there is indeed complexity in the generate_classes pytest fixture, which is built to feed the input in the generator under test by intercepting and mocking the loaders, and then intercept the relevant output data fed to the Renderer in the actual code.
There was a problem hiding this comment.
Oh, right, I didn't realize that that's what ipa= was doing. Doing that renaming would indeed be clearer, yes. I thought that this PR did the complete renaming, but apparently it's backwards-compatible with ipa, so it's probably also fine as it is.
I don't think this is necessary when no actual |
|
sorry about the delay @d10c! I've done the renaming (which was just a matter of |
|
I think it's ok to merge it with your approval. This PR in itself introduces no changes in actually generated code, so it's 0 risk. If when using this feature you'll find issues, we can come back to it with follow up changes. |
This can serve as a basis for #13240.