ugh - mess. maybe we should just keep the add/remove symmetry and deprecate FEATURE_META_PROPERTY then.
On Sat, Nov 28, 2015 at 4:11 PM, Jonathan Ellithorpe <[email protected]> wrote: > 1) Yes, I can submit a PR for fixing the SIMPLE feature requirement set. > 2) I also agree with deprecating > VertexPropertyFeatures.FEATURE_ADD_PROPERTY, but looking at the code I > think I realized there is a slight complication here. That is, what to do > with VertexPropertyFeatures.FEATURE_REMOVE_PROPERTY. Does > VertexFeatures.FEATURE_META_PROPERTIES imply both ADD and REMOVE, or only > ADD? In the later case, we would need to leave > VertexPropertyFeatures.FEATURE_REMOVE_PROPERTIES. Personally, seeing as how > VertexFeatures, extending ElementFeatures, has a FEATURE_ADD_PROPERTY and > FEATURE_REMOVE_PROPERTY, that the FEATURE_META_PROPERTIES be changed to > FEATURE_ADD_METAPROPERTY and FEATURE_REMOVE_METAPROPERTY. > > Jonathan > > On Fri, Nov 27, 2015 at 4:55 AM Stephen Mallette <[email protected]> > wrote: > > > ...damn - hot key sent my post too soon - trying again: > > > > Hi Jonathan, thanks for bringing this up. It would be nice if we could > > expand coverage of our test suite by simply improving the way in which > > features are applied. I was about to suggest a big set of changes when I > > realized that FeatureRequirementSet.SIMPLE is just defined wrong. It > > shouldn't have this entry: > > > > > > > addFeatureRequirement.Factory.create(Graph.Features.VertexPropertyFeatures.FEATURE_ADD_PROPERTY, > > Graph.Features.VertexPropertyFeatures.class)); > > > > it should just be: > > > > > > > add(FeatureRequirement.Factory.create(Graph.Features.VertexFeatures.FEATURE_ADD_PROPERTY, > > Graph.Features.VertexFeatures.class)); > > > > I've created an issue for that to track things: > > > > https://issues.apache.org/jira/browse/TINKERPOP3-997 > > > > because it is a "breaking" change as it will open up tests and possibly > > cause existing implementations to fail. If you'd like to submit a PR for > > this little fix, as you were the reporter for it and as someone who can > > test it in a way that is currently failing for them, just let me know. > > > > As for the this issue: > > Graph.Features.VertexPropertyFeatures.FEATURE_ADD_PROPERTY > > <==> Graph.Features.VertexFeatures.FEATURE_META_PROPERTIES - yeah - we > need > > to deprecate one of those as they are the same thing. Not sure if anyone > > has any preferences on that. in one sense, FEATURE_ADD_PROPERTY is > better > > because it matches the approach for Vertex/Edge. > > On the other hand, the > > documentation refers to this feature as "meta-properties". I guess i > would > > go with keeping FEATURE_META_PROPERTIES and deprecating > > FEATURE_ADD_PROPERTY. I've created an issue as such: > > > > https://issues.apache.org/jira/browse/TINKERPOP3-998 > > If no one has any objections in the next 72 hours (Monday, November 30, > > 2015 at 7:45am) I'll assume lazy consensus and we can move forward with > > this one. > > > > > > > > On Fri, Nov 27, 2015 at 7:35 AM, Stephen Mallette <[email protected]> > > wrote: > > > > > Hi Jonathan, thanks for bringing this up. It would be nice if we could > > > expand coverage of our test suite by simply improving the way in which > > > features are applied. I was about to suggest a big set of changes > when I > > > realized that FeatureRequirementSet.SIMPLE is just defined wrong. It > > > shouldn't have > > > > > > > > > addFeatureRequirement.Factory.create(Graph.Features.VertexPropertyFeatures.FEATURE_ADD_PROPERTY, > > > Graph.Features.VertexPropertyFeatures.class)); > > > > > > it should just be: > > > > > > > > > On Mon, Nov 23, 2015 at 7:39 PM, Jonathan Ellithorpe < > > [email protected]> > > > wrote: > > > > > >> Hello all, > > >> > > >> I am currently working on an experimental implementation of TinkerPop3 > > on > > >> an in-memory key-value store called RAMCloud. In the process of > running > > >> the > > >> unit tests I noticed that turning on support for persistence did not > > >> trigger any new unit tests in GraphTests. Looking into the matter, I > > found > > >> that the unit test that tests this, shouldPersistOnClose, was not > > >> executing > > >> because meta properties support is included in its feature > requirements, > > >> but I do not have support for meta properties. Oddly, though, this > > >> features > > >> requirement seems to be superfluous, since the test does not utilize > > meta > > >> properties. > > >> > > >> An orthogonal issue seems to be that > > >> Graph.Features.VertexPropertyFeatures.FEATURE_ADD_PROPERTY <==> > > >> Graph.Features.VertexFeatures.FEATURE_META_PROPERTIES > > >> > > >> Best, > > >> Jonathan > > >> > > > > > > > > >
