You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@daffodil.apache.org by mbeckerle <gi...@git.apache.org> on 2017/10/28 16:16:12 UTC
[GitHub] incubator-daffodil pull request #1: Property Resolution - part of fixing sch...
GitHub user mbeckerle opened a pull request:
https://github.com/apache/incubator-daffodil/pull/1
Property Resolution - part of fixing schema compilation
(This is a test. Code doesn't pass all tests yet - 34 failures. Trying out code-review mechanisms.)
Refactors property resolving onto elementrefs and grouprefs so
as to eliminate use of backrefs when accessing properties.
Removed alignment logic that was getting tripped up.
Converted OOLAGHost, SchemaComponent etc. down to LocalElementBase
into traits so that I can mix them the way I need to so that
Global elements aren't terms, etc.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mbeckerle/incubator-daffodil daffodil-1855-prop-resolve
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-daffodil/pull/1.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 #1
----
commit 614d34fe3e6de4ef1abd77e9b09cf73b024b97bb
Author: mbeckerle <mb...@tresys.com>
Date: 2017-10-23T21:50:21Z
Property Resolution - part of fixing schema compilation
Refactors property resolving onto elementrefs and grouprefs so
as to eliminate use of backrefs when accessing properties.
Removed alignment logic that was getting tripped up.
Converted OOLAGHost, SchemaComponent etc. down to LocalElementBase
into traits so that I can mix them the way I need to so that
Global elements aren't terms, etc.
----
---
[GitHub] incubator-daffodil pull request #1: Property Resolution - part of fixing sch...
Posted by stevedlawrence <gi...@git.apache.org>.
Github user stevedlawrence commented on a diff in the pull request:
https://github.com/apache/incubator-daffodil/pull/1#discussion_r148030139
--- Diff: daffodil-core/src/main/scala/edu/illinois/ncsa/daffodil/dpath/Expression.scala ---
@@ -294,8 +294,8 @@ case class ComparisonExpression(op: String, adds: List[Expression])
case ("=", _) => subsetError("Unsupported operation '%s'. Use 'eq' instead.", op)
case ("!=", _) => subsetError("Unsupported operation '%s'. Use 'ne' instead.", op)
- case("eq", HexBinary) => EQ_CompareByteArray
- case("ne", HexBinary) => NE_CompareByteArray
+ case ("eq", HexBinary) => EQ_CompareByteArray
+ case ("ne", HexBinary) => NE_CompareByteArray
--- End diff --
Testing commenting on a code hunk.
---
[GitHub] incubator-daffodil issue #1: Property Resolution - part of fixing schema com...
Posted by mbeckerle <gi...@git.apache.org>.
Github user mbeckerle commented on the issue:
https://github.com/apache/incubator-daffodil/pull/1
Not merging. This was a test pull request.
---
[GitHub] incubator-daffodil pull request #1: Property Resolution - part of fixing sch...
Posted by mbeckerle <gi...@git.apache.org>.
Github user mbeckerle closed the pull request at:
https://github.com/apache/incubator-daffodil/pull/1
---
[GitHub] incubator-daffodil issue #1: Property Resolution - part of fixing schema com...
Posted by stevedlawrence <gi...@git.apache.org>.
Github user stevedlawrence commented on the issue:
https://github.com/apache/incubator-daffodil/pull/1
Testing general comment.
---
[GitHub] incubator-daffodil pull request #1: Property Resolution - part of fixing sch...
Posted by mbeckerle <gi...@git.apache.org>.
Github user mbeckerle commented on a diff in the pull request:
https://github.com/apache/incubator-daffodil/pull/1#discussion_r148041815
--- Diff: daffodil-core/src/main/scala/edu/illinois/ncsa/daffodil/compiler/Compiler.scala ---
@@ -67,6 +67,7 @@ import edu.illinois.ncsa.daffodil.processors.parsers.NotParsableParser
import edu.illinois.ncsa.daffodil.processors.unparsers.NotUnparsableUnparser
--- End diff --
This is to figure out the difference between a review, and just comments on a patch set.
---
[GitHub] incubator-daffodil pull request #1: Property Resolution - part of fixing sch...
Posted by VTGuy <gi...@git.apache.org>.
Github user VTGuy commented on a diff in the pull request:
https://github.com/apache/incubator-daffodil/pull/1#discussion_r148033757
--- Diff: daffodil-core/src/main/scala/edu/illinois/ncsa/daffodil/dpath/Expression.scala ---
@@ -294,8 +294,8 @@ case class ComparisonExpression(op: String, adds: List[Expression])
case ("=", _) => subsetError("Unsupported operation '%s'. Use 'eq' instead.", op)
case ("!=", _) => subsetError("Unsupported operation '%s'. Use 'ne' instead.", op)
- case("eq", HexBinary) => EQ_CompareByteArray
- case("ne", HexBinary) => NE_CompareByteArray
+ case ("eq", HexBinary) => EQ_CompareByteArray
+ case ("ne", HexBinary) => NE_CompareByteArray
--- End diff --
Making a **bold** comment. :1st_place_medal:
---
Re: [GitHub] incubator-daffodil pull request #1: Property Resolution
- part of fixing sch...
Posted by Steve Lawrence <sl...@apache.org>.
This all seems reasonable to me. One thing I noticed though is that
there is no ability to accept the pull request and merge it in via the
github interface as described in our workflow [1]. I think this
functionality is enabled by enabling Apache GitBox. I'll open an infra
ticket to enable GitBox for Daffodil, but I think we all also need to
link our Apache and GitHub accounts via:
https://gitbox.apache.org/setup/
- Steve
[1]
https://cwiki.apache.org/confluence/display/DAFFODIL/Code+Contributor+Workflow
On 10/31/2017 11:23 AM, mbeckerle wrote:
> Github user mbeckerle commented on a diff in the pull request:
>
> https://github.com/apache/incubator-daffodil/pull/1#discussion_r148030522
>
> --- Diff: daffodil-core/src/main/scala/edu/illinois/ncsa/daffodil/dpath/Expression.scala ---
> @@ -294,8 +294,8 @@ case class ComparisonExpression(op: String, adds: List[Expression])
> case ("=", _) => subsetError("Unsupported operation '%s'. Use 'eq' instead.", op)
> case ("!=", _) => subsetError("Unsupported operation '%s'. Use 'ne' instead.", op)
>
> - case("eq", HexBinary) => EQ_CompareByteArray
> - case("ne", HexBinary) => NE_CompareByteArray
> + case ("eq", HexBinary) => EQ_CompareByteArray
> + case ("ne", HexBinary) => NE_CompareByteArray
> --- End diff --
>
> Thank you for that useful comment. (Just a test of comments and replies while I figure out this code-review system.)
>
>
> ---
>
Re: [GitHub] incubator-daffodil pull request #1: Property Resolution
- part of fixing sch...
Posted by Steve Lawrence <sl...@apache.org>.
Agreed. It looks like GitHub really just provide the "Jump To" thing to
pick files, but that isn't nearly convenient.
It looks like the OctoTree browser extension [1] can add a tree view,
somewhat similar to what we had with Fisheye. One thing this lacks that
Fisheye had was that it marked which files you've already looked at.
This doesn't appear to have that, but it does have a nice tree view.
- Steve
[1] https://github.com/buunguyen/octotree
On 10/31/2017 11:35 AM, Mike Beckerle wrote:
> One thing I'm hoping there is a way to do. I'd like to see all the files in a review in a tree view.
>
>
> Using fisheye on NCSA the review window has a tree-view of the complete change set on the left, so you can jump around the change in a logical order based on where and what all the changes are. E.g., you can start from the tests, or jump into the runtime parts, first, then circle back to other parts, etc.
>
>
> I am hoping there's a way to achieve this using the github infrastructure. Daffodil still gets many changes that involve 60+ files, so that tree view is very important. The basic github view of a patch set seems to be oriented toward small changes that touch only a small number of files.
>
>
> The alternative I know of is to pull the branch into one's local repo. Then you can view the change using an IDE. That misses out on the ability to rapidly comment on it, and muddies the waters of what has changed, vs. what has stayed the same; however, it does also help very much with navigating a large patch set as one uses the same tools one uses when programming/debugging.
>
>
> Is there a third (fourth, fifth...) way to do this that would be more like the fisheye experience, i.e., a middle ground between just file-by file patches, and pulling the branch and using ordinary IDE?
>
>
>
> ________________________________
> From: mbeckerle <gi...@git.apache.org>
> Sent: Tuesday, October 31, 2017 11:23:41 AM
> To: dev@daffodil.incubator.apache.org
> Subject: [GitHub] incubator-daffodil pull request #1: Property Resolution - part of fixing sch...
>
> Github user mbeckerle commented on a diff in the pull request:
>
> https://github.com/apache/incubator-daffodil/pull/1#discussion_r148030522
>
> --- Diff: daffodil-core/src/main/scala/edu/illinois/ncsa/daffodil/dpath/Expression.scala ---
> @@ -294,8 +294,8 @@ case class ComparisonExpression(op: String, adds: List[Expression])
> case ("=", _) => subsetError("Unsupported operation '%s'. Use 'eq' instead.", op)
> case ("!=", _) => subsetError("Unsupported operation '%s'. Use 'ne' instead.", op)
>
> - case("eq", HexBinary) => EQ_CompareByteArray
> - case("ne", HexBinary) => NE_CompareByteArray
> + case ("eq", HexBinary) => EQ_CompareByteArray
> + case ("ne", HexBinary) => NE_CompareByteArray
> --- End diff --
>
> Thank you for that useful comment. (Just a test of comments and replies while I figure out this code-review system.)
>
>
> ---
>
Re: [GitHub] incubator-daffodil pull request #1: Property Resolution
- part of fixing sch...
Posted by Mike Beckerle <mb...@tresys.com>.
One thing I'm hoping there is a way to do. I'd like to see all the files in a review in a tree view.
Using fisheye on NCSA the review window has a tree-view of the complete change set on the left, so you can jump around the change in a logical order based on where and what all the changes are. E.g., you can start from the tests, or jump into the runtime parts, first, then circle back to other parts, etc.
I am hoping there's a way to achieve this using the github infrastructure. Daffodil still gets many changes that involve 60+ files, so that tree view is very important. The basic github view of a patch set seems to be oriented toward small changes that touch only a small number of files.
The alternative I know of is to pull the branch into one's local repo. Then you can view the change using an IDE. That misses out on the ability to rapidly comment on it, and muddies the waters of what has changed, vs. what has stayed the same; however, it does also help very much with navigating a large patch set as one uses the same tools one uses when programming/debugging.
Is there a third (fourth, fifth...) way to do this that would be more like the fisheye experience, i.e., a middle ground between just file-by file patches, and pulling the branch and using ordinary IDE?
________________________________
From: mbeckerle <gi...@git.apache.org>
Sent: Tuesday, October 31, 2017 11:23:41 AM
To: dev@daffodil.incubator.apache.org
Subject: [GitHub] incubator-daffodil pull request #1: Property Resolution - part of fixing sch...
Github user mbeckerle commented on a diff in the pull request:
https://github.com/apache/incubator-daffodil/pull/1#discussion_r148030522
--- Diff: daffodil-core/src/main/scala/edu/illinois/ncsa/daffodil/dpath/Expression.scala ---
@@ -294,8 +294,8 @@ case class ComparisonExpression(op: String, adds: List[Expression])
case ("=", _) => subsetError("Unsupported operation '%s'. Use 'eq' instead.", op)
case ("!=", _) => subsetError("Unsupported operation '%s'. Use 'ne' instead.", op)
- case("eq", HexBinary) => EQ_CompareByteArray
- case("ne", HexBinary) => NE_CompareByteArray
+ case ("eq", HexBinary) => EQ_CompareByteArray
+ case ("ne", HexBinary) => NE_CompareByteArray
--- End diff --
Thank you for that useful comment. (Just a test of comments and replies while I figure out this code-review system.)
---
[GitHub] incubator-daffodil pull request #1: Property Resolution - part of fixing sch...
Posted by mbeckerle <gi...@git.apache.org>.
Github user mbeckerle commented on a diff in the pull request:
https://github.com/apache/incubator-daffodil/pull/1#discussion_r148030522
--- Diff: daffodil-core/src/main/scala/edu/illinois/ncsa/daffodil/dpath/Expression.scala ---
@@ -294,8 +294,8 @@ case class ComparisonExpression(op: String, adds: List[Expression])
case ("=", _) => subsetError("Unsupported operation '%s'. Use 'eq' instead.", op)
case ("!=", _) => subsetError("Unsupported operation '%s'. Use 'ne' instead.", op)
- case("eq", HexBinary) => EQ_CompareByteArray
- case("ne", HexBinary) => NE_CompareByteArray
+ case ("eq", HexBinary) => EQ_CompareByteArray
+ case ("ne", HexBinary) => NE_CompareByteArray
--- End diff --
Thank you for that useful comment. (Just a test of comments and replies while I figure out this code-review system.)
---
[GitHub] incubator-daffodil pull request #1: Property Resolution - part of fixing sch...
Posted by mbeckerle <gi...@git.apache.org>.
Github user mbeckerle commented on a diff in the pull request:
https://github.com/apache/incubator-daffodil/pull/1#discussion_r148041559
--- Diff: daffodil-core/src/main/scala/edu/illinois/ncsa/daffodil/compiler/Compiler.scala ---
@@ -67,6 +67,7 @@ import edu.illinois.ncsa.daffodil.processors.parsers.NotParsableParser
import edu.illinois.ncsa.daffodil.processors.unparsers.NotUnparsableUnparser
--- End diff --
Just using to try the "start a review" button, which is grey until you type a comment.
---