Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-997

FeatureRequirementSet.SIMPLE should not require multi-property

    Details

      Description

      The intention was not to include multi-property - it was supposed to just allow properties on vertices which is a pretty low bar. Includes the "breaking" label because it will open up tests for some graphs and could cause them to have failing tests.

        Activity

        Hide
        Ellitron Jonathan Ellithorpe added a comment -

        I suppose another way to put this is that SIMPLE should never be applied to tests which do not exercise all of the features requirements in SIMPLE. The feature requirements for a test should always exactly match the features used by the test.

        Show
        Ellitron Jonathan Ellithorpe added a comment - I suppose another way to put this is that SIMPLE should never be applied to tests which do not exercise all of the features requirements in SIMPLE. The feature requirements for a test should always exactly match the features used by the test.
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user spmallette opened a pull request:

        https://github.com/apache/incubator-tinkerpop/pull/205

        TINKERPOP-997 - Feature and FeatureRequirement adjustments

        As they were very closely related, this PR covers:

        https://issues.apache.org/jira/browse/TINKERPOP-997
        https://issues.apache.org/jira/browse/TINKERPOP-998

        it deprecates a `Feature` that was basically a duplicate of another. It also opens up a good number of test cases to graphs that don't support meta-properties as there was a mis-definition of the `SIMPLE` requirement set.

        Tested with `mvn clean install -DincludeNeo4j` and all was good. Also temporarily modified the TinkerGraph feature for meta-property support and manually noted that the right test cases opened up and closed down based on this change. Would be nice to get some verification from other graphs that this change is all good.

        @jdellithorpe please take a look at this change when you get a moment, as you were the one who noted the problem in the first place. Looks like we get about 90 tests opened up as a result of this.

        VOTE +1

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-997

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/incubator-tinkerpop/pull/205.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #205


        commit cc6f10dbee050136729a407dd50b2c123729ebdb
        Author: Stephen Mallette <spmva@genoprime.com>
        Date: 2016-01-19T17:53:56Z

        TINKERPOP-998 Deprecated the VertexPropertyFeatures.FEATURE_ADD_PROPERTY.

        Replaced by the already existing (duplicate) VertexFeatures.FEATURE_META_PROPERTIES.

        commit 74cd88cede4439a93ec764fce623d4a499052336
        Author: Stephen Mallette <spmva@genoprime.com>
        Date: 2016-01-19T18:58:28Z

        TINKERPOP-997 FeatureRequirementSet.SIMPLE uses VertexFeatures.FEATURE_ADD_PROPERTY

        This opens up a large number of tests in the test suite (about 90 or so) - providers may see some breakage as a result.


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/incubator-tinkerpop/pull/205 TINKERPOP-997 - Feature and FeatureRequirement adjustments As they were very closely related, this PR covers: https://issues.apache.org/jira/browse/TINKERPOP-997 https://issues.apache.org/jira/browse/TINKERPOP-998 it deprecates a `Feature` that was basically a duplicate of another. It also opens up a good number of test cases to graphs that don't support meta-properties as there was a mis-definition of the `SIMPLE` requirement set. Tested with `mvn clean install -DincludeNeo4j` and all was good. Also temporarily modified the TinkerGraph feature for meta-property support and manually noted that the right test cases opened up and closed down based on this change. Would be nice to get some verification from other graphs that this change is all good. @jdellithorpe please take a look at this change when you get a moment, as you were the one who noted the problem in the first place. Looks like we get about 90 tests opened up as a result of this. VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-997 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tinkerpop/pull/205.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #205 commit cc6f10dbee050136729a407dd50b2c123729ebdb Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-01-19T17:53:56Z TINKERPOP-998 Deprecated the VertexPropertyFeatures.FEATURE_ADD_PROPERTY. Replaced by the already existing (duplicate) VertexFeatures.FEATURE_META_PROPERTIES. commit 74cd88cede4439a93ec764fce623d4a499052336 Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-01-19T18:58:28Z TINKERPOP-997 FeatureRequirementSet.SIMPLE uses VertexFeatures.FEATURE_ADD_PROPERTY This opens up a large number of tests in the test suite (about 90 or so) - providers may see some breakage as a result.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user okram commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/205#issuecomment-173977199

        VOTE +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/205#issuecomment-173977199 VOTE +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user twilmes commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/205#issuecomment-173979930

        Looks good. VOTE +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user twilmes commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/205#issuecomment-173979930 Looks good. VOTE +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/incubator-tinkerpop/pull/205

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/incubator-tinkerpop/pull/205
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user dkuppitz commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/205#issuecomment-174044610

        Just to make that time spent on integration tests not too worthless:

        • `mvn clean install`: passed
        • integration tests: passed

        VOTE: +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/205#issuecomment-174044610 Just to make that time spent on integration tests not too worthless: `mvn clean install`: passed integration tests: passed VOTE: +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/205#issuecomment-174046598

        thanks @dkuppitz - the real test will be when providers start running their tests and stuff starts popping off. hopefully a few get a chance next week to try out all the tests that opened up as a result of these changes.

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/205#issuecomment-174046598 thanks @dkuppitz - the real test will be when providers start running their tests and stuff starts popping off. hopefully a few get a chance next week to try out all the tests that opened up as a result of these changes.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jdellithorpe commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/205#issuecomment-174620611

        This looks good to me. Just to be clear, now supporting meta-properties means specifically supporting the addition of meta-properties. Removing meta-properties is a separate feature, not implied by FEATURE_META_PROPERTIES.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jdellithorpe commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/205#issuecomment-174620611 This looks good to me. Just to be clear, now supporting meta-properties means specifically supporting the addition of meta-properties. Removing meta-properties is a separate feature, not implied by FEATURE_META_PROPERTIES.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/205#issuecomment-174964582

        yeah - we still have `VertexPropertyFeatures.supportsRemoveProperty()` hanging out there.

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/205#issuecomment-174964582 yeah - we still have `VertexPropertyFeatures.supportsRemoveProperty()` hanging out there.

          People

          • Assignee:
            spmallette stephen mallette
            Reporter:
            spmallette stephen mallette
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development