Skip to content
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

Jump to bottom

~ migrate ShExJ to top-level ShapeDecls #51

Merged
merged 5 commits into from
Jul 6, 2022
Merged

~ migrate ShExJ to top-level ShapeDecls #51

merged 5 commits into from
Jul 6, 2022

Conversation

ericprud
Copy link
Contributor

@ericprud ericprud force-pushed the extends_top-level-ShapeDecl branch 7 times, most recently from 3ea46c8 to e689670 Compare June 16, 2022 09:13
Eric Prud'hommeaux added 2 commits June 16, 2022 11:13
~ migrate ShExJ to top-level ShapeDecls
05076ea 
per #50

test with:
  (ls schemas/*.json | grep -vE '(coverage|representationTests)\.json' | xargs ~/checkouts/ericprud/jsg/lib/cli.js doc/ShExJ.jsg)
~ migrate ShExR to top-level ShapeDecls
ae47b86 
per #50

test with:
  for f in $(ls schemas/*.ttl | grep -vE '(manifest|meta)\.ttl'); do echo $f && ~/checkouts/shexSpec/shex.js/packages/shex-cli/bin/validate -m '{FOCUS a sx:Schema}@sx:Schema' -x doc/ShExR.shex -d $f > /dev/null || echo $f; done

TODO:
* schemas/1valExprRefbnode-IV1.ttl
* schemas/1valExprRef-IV1.ttl
* schemas/2RefS1-Icirc.ttl
* schemas/2RefS1-IS2.ttl
* schemas/3circRefS12.ttl
* schemas/3circRefS1-Icirc.ttl
* schemas/3circRefS1-IS23.ttl
* schemas/3circRefS1-IS2-IS3-IS3.ttl
* schemas/3circRefS1-IS2-IS3.ttl
* schemas/3circRefS23.ttl
* schemas/3circRefS2-Icirc.ttl
* schemas/3circRefS2-IS3.ttl
* schemas/3circRefS3-Icirc.ttl
* schemas/3circRefS3-IS12.ttl
* schemas/3circRefS3.ttl
* schemas/start2RefS1-IstartS2.ttl
* schemas/start2RefS2-IstartS1.ttl
@ericprud ericprud force-pushed the extends_top-level-ShapeDecl branch from e689670 to ae47b86 Compare June 16, 2022 09:14
@ericprud ericprud marked this pull request as ready for review June 27, 2022 10:24
@ericprud ericprud requested review from gkellogg and labra June 27, 2022 10:25
gkellogg
gkellogg reviewed Jun 27, 2022
View reviewed changes
Copy link
Contributor

@gkellogg gkellogg left a comment

Choose a reason for hiding this comment

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

Are there new tests that show the advantage of this indirection rather than just the extra overhead of indirection?

@ericprud
Copy link
Contributor Author

Are there new tests that show the advantage of this indirection rather than just the extra overhead of indirection?

I don't think the tests show it off. The ShExJ.jsg is a ton simpler.
I think the strongest case comes from diving into an implementation and seeing the number of potential bugs that disappear. There are lots of places where I wasn't conditionally traversing the ShapeDecls's .shexExpr (e.g.) because I hadn't happened to hit those lines with a schema that needed a ShapeDecl (i.e. had .abstract set to true).

@ericprud
Copy link
Contributor Author

ericprud commented Jun 29, 2022 •
edited
Loading

Tx @gkellogg . I'll wait 'till Iovka gets shex-java running with the current extensd tests before pushing this upstream with the idea of making her steps to passing the tests more incremental.
@iovka, that may require you to do extra coding so if you prefer, just you can merge this into extends and and pull the merged extends.

@ericprud
Copy link
Contributor Author

ericprud commented Jul 6, 2022

Labra vocalized his approval in EXTENDS meeting on 2022-07-05

@ericprud ericprud merged commit ce68ab7 into extends Jul 6, 2022
@ericprud ericprud deleted the extends_top-level-ShapeDecl branch July 6, 2022 16:09
@ericprud ericprud mentioned this pull request Jul 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkellogg gkellogg gkellogg approved these changes

@labra labra Awaiting requested review from labra

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ericprud @gkellogg