Skip to content

WIP: resolve property references to other namespace #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fra-pa wants to merge 20 commits into from
Closed

WIP: resolve property references to other namespace #39

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
8b726df
feat: resolve property references to other namespace
fra-pa May 11, 2021
3500f89
test: improve multiple inheritance tests
fra-pa May 19, 2021
c332a7e
test: expect default values to be initialized
fra-pa May 19, 2021
3445726
fix: add default values for properties in other ns
fra-pa May 19, 2021
1b5ce8c
test: fix linting error 'Missing semicolon'
fra-pa May 19, 2021
7bdb05f
test: verify the string thrown by 'getType()'
fra-pa May 20, 2021
a4de9b3
test: expect to not throw any exception
fra-pa May 20, 2021
1af0602
test: expect properties from same ns to be eql
fra-pa May 20, 2021
36aaacc
fix: 'localName' and 'name are equal in 'propertiesByName'
fra-pa May 20, 2021
850e039
chore: fix linting error
fra-pa May 20, 2021
d9edaf2
test: add third 'superClass' and test descriptor
fra-pa May 21, 2021
15287f6
test: move tests 'multiple inherited' to seperate file
fra-pa May 21, 2021
eb02138
test: add local property and verify descriptor
fra-pa May 21, 2021
96a9eab
test: fix call to 'getElementDescriptor'
fra-pa May 21, 2021
073c3e9
test: overwriting local property should throw exception
fra-pa May 21, 2021
e75cba8
feat: revert removal of 'defineProperty()'
fra-pa May 21, 2021
d6f342b
test: expect equality of properties
fra-pa Jun 2, 2021
596d002
test: revert to original formatting
fra-pa May 30, 2021
7b0a6f5
chore: refactor assertNotDefined()
fra-pa Jun 2, 2021
66bb9e0
fix: remove obsolete line to resolve parsing error
fra-pa Jun 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions lib/descriptor-builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,13 @@ DescriptorBuilder.prototype.addNamedProperty = function(p, validate) {
propsByName = this.propertiesByName;

if (validate) {
this.assertNotDefined(p, ns.name);
this.assertNotDefined(p, ns.localName);
this.assertNotDefined(p);
}

propsByName[ns.name] = propsByName[ns.localName] = p;
if (!this.propertiesByName[ns.name])
propsByName[ns.name] = propsByName[ns.localName] = p;
else if (!this.propertiesByName[ns.localName])
propsByName[ns.localName] = p;
};

DescriptorBuilder.prototype.removeNamedProperty = function(p) {
Expand Down Expand Up @@ -172,15 +174,18 @@ DescriptorBuilder.prototype.setIdProperty = function(p, validate) {
this.idProperty = p;
};

DescriptorBuilder.prototype.assertNotDefined = function(p, name) {
var propertyName = p.name,
DescriptorBuilder.prototype.assertNotDefined = function(p) {
var ns = p.ns,
propertyName = p.name,
definedProperty = this.propertiesByName[propertyName];

if (definedProperty) {
if (this.propertiesByName[ns.name] && this.propertiesByName[ns.localName]) {
Comment thread
nikku marked this conversation as resolved.
Outdated
throw new Error(
'property <' + propertyName + '> already defined; ' +
'override of <' + definedProperty.definedBy.ns.name + '#' + definedProperty.ns.name + '> by ' +
'<' + p.definedBy.ns.name + '#' + p.ns.name + '> not allowed without redefines');
'override of <' + definedProperty.definedBy.ns.name + '#' +
Copy link
Copy Markdown
Member

@nikku nikku May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I blind or is this exactly equivalent to the original version, just with a different formatting?

Copy link
Copy Markdown
Contributor Author

@fra-pa fra-pa Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, to make the review harder than necessary with this reformatting. In the course of modifying the function, I used code, dismissed it later and forgot to also revert the formatting.

Your remark brought the function back to my mind. After taking another look, I refactored the variables and changed the error message. I hope this makes the namespace aspect more explicit.

definedProperty.ns.name + '> by ' +
'<' + p.definedBy.ns.name + '#' + p.ns.name +
'> not allowed without redefines');
}
};

Expand Down Expand Up @@ -230,4 +235,4 @@ DescriptorBuilder.prototype.addTrait = function(t, inherited) {

types.push(t);
typesByName[typeName] = t;
};
};
14 changes: 12 additions & 2 deletions lib/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,17 @@ Factory.prototype.createType = function(descriptor) {
// initialize default values
forEach(descriptor.properties, function(p) {
if (!p.isMany && p.default !== undefined) {
prototype[p.name] = p.default;
var name;

if (p.ns.prefix === descriptor.ns.prefix) {

// use local name if property already in local ns
name = p.name;
Copy link
Copy Markdown
Member

@nikku nikku May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting question here: What I'd expect is to have local name and ns name attribute present.

I.e. I can always access the value via foo["a:id"] regardless if it has a local name set or not. On the other hand I'd like to have a#id always available as the shorthand, if it is the only id property defined for an element.

Likely this is only possible to accomplish via proxies, am I right about that?

Copy link
Copy Markdown
Contributor Author

@fra-pa fra-pa May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting question here: What I'd expect is to have local name and ns name attribute present.

They are present, but ...

I.e. I can always access the value via foo["a:id"] regardless if it has a local name set or not. On the other hand I'd like to have a#id always available as the shorthand, if it is the only id property defined for an element.

Likely this is only possible to accomplish via proxies, am I right about that?

For isMany-properties this is possible without a proxy, as we reference an object. But fundamental types (i.e. BUILTINS) are only accessible through get() when referenced with their localName. a#id on the other hand is always available through a.id (and a.get('id')of course), as long it was previously initialized (see my remarks below) - so yes, you're right.

IMO this is a compromise that we should accept. As you say, the alternative would be the introduction of proxy objects.

I hope we understand each other (please don't hesitate to keep raising questions). The test cases should hold the truth to specify desired behaviour. So we need to make sure to get them right and fully cover this topic as well as update the documentation before merging.

Btw, the current implementation also fails to access foo['a:id'] if not initialized during creation or has a default value or was previously accessed with get(). I've not tested this out yet, but this seems to be the case for all properties except foo.id. I.e. you need to use get() first before foo['a:id'] is mapped into the object.

This is somehow inconsistent and it might be a nice improvement to map property instances early (i.e. during type creation) even if they are not explicitly initialized or have a default value. I'd more like to get undefined instead of an unexpected exception. If you like, I'll open another issue for this.

One question in this context: should it be possible to define a default value for isMany-properties (e.g. "default": [ 1, 2 ])? I tried this, received no exception but the value was not applied. Maybe I miss it, but there seems to be no documentation or spec for this. Should I address this in a seperate issue or am I just wrong here?

}
else {
name = p.ns.name;
}
prototype[name] = p.default;
}
});

Expand Down Expand Up @@ -58,4 +68,4 @@ Factory.prototype.createType = function(descriptor) {
props.defineDescriptor(ModdleElement, descriptor);

return ModdleElement;
};
};
44 changes: 31 additions & 13 deletions lib/properties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ export default function Properties(model) {
Properties.prototype.set = function(target, name, value) {

var property = this.model.getPropertyDescriptor(target, name);

var propertyName = property && property.name;
var propertyName = getPropertyName(target, name, property);

if (isUndefined(value)) {
// unset the property, if the specified value is undefined;
Expand All @@ -37,14 +36,20 @@ Properties.prototype.set = function(target, name, value) {
if (propertyName in target) {
target[propertyName] = value;
} else {
defineProperty(target, property, value);
Object.defineProperty(target, propertyName, {
enumerable: !property.isReference,
writable: true,
value: value,
configurable: true
});
}
} else {
target.$attrs[name] = value;
}
}
};


/**
* Returns the named property of the given element
*
Expand All @@ -61,11 +66,16 @@ Properties.prototype.get = function(target, name) {
return target.$attrs[name];
}

var propertyName = property.name;
var propertyName = getPropertyName(target, name, property);

// check if access to collection property and lazily initialize it
if (!target[propertyName] && property.isMany) {
defineProperty(target, property, []);
Object.defineProperty(target, propertyName, {
Comment thread
nikku marked this conversation as resolved.
Outdated
enumerable: !property.isReference,
writable: true,
value: [],
configurable: true
});
}

return target[propertyName];
Expand Down Expand Up @@ -103,11 +113,19 @@ function isUndefined(val) {
return typeof val === 'undefined';
}

function defineProperty(target, property, value) {
Object.defineProperty(target, property.name, {
enumerable: !property.isReference,
writable: true,
value: value,
configurable: true
});
}

function getPropertyName(target, name, descriptor) {
if (!target || !name || !descriptor)
return undefined;

var propertyName = descriptor.name;

if (name.includes(':')) {
var parts = name.split(':');

if (parts[0] !== target.$descriptor.ns.prefix)
propertyName = descriptor.ns.name;
}

return propertyName;
}
37 changes: 37 additions & 0 deletions test/fixtures/model/multiple-inherited-properties.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
{
"name": "MultipleInheritance",
"uri": "http://multipleinheritance",
"prefix": "mh",
"types": [
{
"name": "AnotherRoot",
"properties": [
{
"name": "any",
"type": "AnotherRoot",
"isMany": true
},
{
"name": "many",
"type": "String",
"isMany": true
},
{
"name": "single",
"type": "String"
},
{
"name": "defaultSingle",
"type": "String",
"default": "mh-default-string"
}
]
},
{
"name": "MultipleInherited",
"superClass": [ "AnotherRoot", "props:Root" ],
"properties": [
]
}
]
}
23 changes: 22 additions & 1 deletion test/fixtures/model/properties.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@
{ "name": "idNumeric", "type": "Integer", "isAttr": true, "redefines": "BaseWithId#id", "isId": true }
]
},
{
"name": "BaseWithAlreadyDefinedId",
"superClass": [ "BaseWithId" ],
"properties": [
{ "name": "id", "type": "Integer", "isAttr": true, "isId": true }
]
},
{
"name": "Attributes",
"superClass": [ "BaseWithId" ],
Expand Down Expand Up @@ -122,6 +129,20 @@
"name": "any",
"type": "Base",
"isMany": true
},
Comment thread
nikku marked this conversation as resolved.
{
"name": "many",
"type": "Integer",
"isMany": true
},
{
"name": "single",
"type": "Integer"
},
{
"name": "defaultSingle",
"type": "Integer",
"default": 42
}
]
},
Expand Down Expand Up @@ -171,4 +192,4 @@
]
}
]
}
}
Loading