Create Builder DSL, and add X212-HN277 Parser With It #200

AnthonySuper · 2019-07-18T17:49:52Z

This pull request does two things.

Builder DSL

The first thing it provides is a "builder DSL", an alternative style of defining document formats.
Currently, documents are defined by building the AST with a series of method calls.
This works out okay, but it's a little harder to read and write, in my opinion.
The builder DSL uses blocks instead, so that you can write something like this:

segment(100, s::HL, "Information Source Level", r::Required, d::RepeatCount.bounded(1)) do
  element(e::Required,    "Hierarchical ID Number")
  element(e::NotUsed,     "Hierarchical Parent ID Number")
  element(e::Required,    "Hierarchical Level Code", values("20"))
  element(e::Required,    "Hierachical Child Code", values("1"))
end

This style made it a lot easier for me to write the document definition, and I think it could be a valuable contribution to this gem.
I hope that the second part of the PR can demonstrate its utility by actually using it to define a new document format.

X212-HN277 Parsing

The other component of this PR is parsing for 005010 X212 HN-277 documents.
We get a lot of these at the company I'm opening this pull request on behalf of, and would like to use this gem to ingest them.
So I wrote the code to enable us to do that, using the new DSL definition style.

Final Thoughts

If you have any requested changes, additional specs, or what have you, please let me know!
Stupidedi has saved us a ton of work at Sondermind, and we want to give back and improve the gem.

Creating the AST via method calls is a little difficult to read. This provides a more ruby-ish DSL interface to define your AST.

This parses a new format of 277 document. It also makes use of the new DSL.

This commit provides slightly nicer comments and variable naming for the DSL, as well as a fix for the X212HN277 parsing.

kputnam · 2019-07-30T19:46:49Z

Hey @AnthonySuper, this looks really nice! I think this should be merged, but I will need a little time to give it a closer look. I thought we had a definition for HN277 already, but I see it's for X214 instead of X212, so thanks for contributing that too!

AnthonySuper · 2019-07-31T18:56:39Z

@kputnam Thank you! If you want any changes, let me know.

AnthonySuper · 2019-09-03T15:15:27Z

@kputnam Hate to be a bother, but did you get a chance to take a further look at this?

kputnam · 2019-09-09T21:55:29Z

spec/lib/stupidedi/transaction_sets/005010/X212-HN277_spec.rb

+      end
+    end
+
+    describe "structure" do


This spec isn't needed -- a mostly equivalent spec is automatically generated by virtue of having your fixture file matching the naming convention. See this for details. To be certain it's testing your fixture, you can move the spec from pass/ to fail/ (or alternatively, edit the fixture to introduce a syntax error), then see that the spec fails.

However, you can still add specs here similar to this which ensures your grammar lets you navigate correctly. Doing so can potentially catch errors like this -- that comment shows a quick way to check the grammar, but it's not automated.

If you don't want to create a test that uses have_structure, you can delete this spec file.

Do you mean the entire spec file is redundant, or just this particular structure testing section?

I think both are redundant now that you mention it. Parts of these are spread over different tests elsewhere, though I might need to review the other specs to be certain. Either way, I intend for these two tests to be boilerplate that you don't have to write.

Gotcha. I'll delete the file.

kputnam · 2019-09-09T22:02:05Z

lib/stupidedi/transaction_sets/builder/dsl.rb

+        class LoopBodyDSL
+
+          include LoopSyntax
+          include SegmentSyntax


I like the approach being used here, nice work!

kputnam

Looks good to merge. The coding style has a few more blank lines than I would like, but that's just a nitpick that can be addressed later without much effort. Thanks for this @AnthonySuper!

kputnam · 2019-09-09T22:05:59Z

@irobayna I marked it approved, but I guess we should wait until @AnthonySuper addresses the comment about the redundant spec file.

This is just boilerplate that is pointless to write, so we can safely get rid of it.

AnthonySuper · 2019-09-12T15:07:45Z

Okay, I removed the spec. Some of the tests are still failing on CI (and on my own local build), but the failures all seem to be related to other parts of the codebase. Any idea what might be going on there?

kputnam · 2019-09-12T22:51:15Z

It looks like it's failing on a merge commit f9868e7 -- that looks like your change to remove the spec + the most recent commit from master? I'm not sure but maybe Travis CI tests against your branch with master merged into it, which caused a failure.

Can you merge master into your branch locally and be sure the tests past? It was complaining about NoSuchMethod: repeat_bounded, which appears in dsl_spec but doesn't appear to be defined.

A previous version of the DSL had a syntax for repeat counts, but I removed it because it wound up not actually working very well. This commit updates the spec to reflect that.

AnthonySuper · 2019-09-13T15:08:24Z

Okay, I fixed that spec issue. Now the problem seems to be that it's stepping on the toes of previously-existing sample documents, which is a bit concerning. I'm not sure how the fixtures work, exactly, and to be honest the code that loads them is a little intimidating. I think it's currently testing things that should run against the 214 version against the 212 version? But I'm not sure. I'll take a look at it later.

kputnam · 2019-09-13T19:06:26Z

One thing I noticed is your fixture is in " spec/fixtures/005010/X212 HR277 Heatlh Care Information Status Notification" but I think you want HN277?

That shouldn't have caused the build failure (it should have just ignored those fixtures since the directory name maps to TransactionSets::FiftyTen::Implementations::X212::HR277 that doesn't exist).

You make a good point that code is difficult to understand. I'll make a note to add some commentary and maybe rewrite some of it to be clearer.

kputnam · 2019-09-13T19:14:02Z

Ohhhh I know what's going on. Your transaction set is being applied to previously existing fixtures. Even though I didn't have the X212 HN277 specifications, I think X12 posted example files on their website.

So the build error is saying those files aren't valid according to your grammar. Sometimes X12 publishes a few different iterations of the same grammar (A1, A2, etc for amendments, or E1, E2 for eratta). They could have posted invalid example files too.

But you should double-check that the errors are legitimate. Many of the errors say the 2200B loop is required due to TRN being required (according to your grammar), but those fixture files don't have it. Maybe it's actually not supposed to be required?

The other error was I think your grammar declares STC03 as required, but one of the fixtures has it blank. I'm not sure which instance of STC it's complaining about, but you can tell by using edi-pp to print out the parse tree of that fixture.

AnthonySuper · 2019-09-16T14:32:50Z

Yep, upon further inspection my code has some bugs in it! Unfortunately I wrote this a while back so I don't have an excuse for them handy.

I'll fix these as soon as I get the time. Unfortunately we have an upcoming feature deadline on Monday here, so I probably won't get to them this week, but the week after that I believe I can prioritize this.

My apologies for submitting a buggy parser! I tested this primarily against our own documents, which tend to have more stuff in them than the minimal examples. I'll get it fixed ASAP.

kputnam · 2019-09-16T18:47:25Z

No problem! I know how that goes... there is a lot of stuff I don't remember writing here too. Feel free to take as much time as you like.

AnthonySuper · 2019-10-01T17:15:55Z

@kputnam Okay! The parser is now fixed and passes specs correctly. My apologies for initially submitting a bad one.

ram-nadella · 2020-07-18T03:57:32Z

We have a use case where we need to write a schema to support a non-standard implementation of an EDI file. Bringing the DSL style would be a nice addition to the already super useful library.

@AnthonySuper @kputnam @irobayna anything I can do to help bring it in?

Looks like the tests are passing for most ruby versions but failing in these environments: Ruby 2.1.1, Ruby 2.1.2 and Rubt 2.7.0-preview1. Haven't looked into the details yet, but perhaps we could drop ruby 2.1.1 and 2.1.2? Thoughts? These versions are quite old and unsupported, tests are passing on 2.1.5 - 2.1.8, which means the library would still be tested against the 2.1.x line.

kputnam · 2020-07-20T18:48:57Z

@ram-nadella Thanks for reminding me about this. I'll take a look again today. I generally support any versions of Ruby that Travis CI will run (even if they are no longer maintained, i.e. Ruby <= 2.4), but it looks like Ruby 2.1.3-5 no longer have binaries available. Also 2.7 has been out for a while, so I'll see if our build passes on the newer versions.

kputnam · 2020-07-21T04:01:46Z

I restarted the build after updating master to exclude the problematic Ruby versions. But I guess Travis didn't merge master in because it still builds against 2.1.3-5, and somehow it passes(!?).

Going to merge this and if you need us to release a gem, let us know. Otherwise it'll be available in master until we make the next release.

AnthonySuper · 2020-07-21T15:49:27Z

Awesome! Thanks for merging this.

Anthony Super added 3 commits July 17, 2019 10:00

Add Builder DSL

499e613

Creating the AST via method calls is a little difficult to read. This provides a more ruby-ish DSL interface to define your AST.

Implement 005010X212-277 Parsing

80ede17

This parses a new format of 277 document. It also makes use of the new DSL.

Cleanup + Documentation for DSL

6f59448

This commit provides slightly nicer comments and variable naming for the DSL, as well as a fix for the X212HN277 parsing.

irobayna requested review from kputnam and irobayna July 18, 2019 20:11

kputnam reviewed Sep 9, 2019

View reviewed changes

kputnam approved these changes Sep 9, 2019

View reviewed changes

Remove Unused Spec File

475c141

This is just boilerplate that is pointless to write, so we can safely get rid of it.

Anthony Super added 2 commits September 13, 2019 07:55

Merge remote-tracking branch 'upstream/master'

00850b4

Fix Repeat in Spec

6541cdb

A previous version of the DSL had a syntax for repeat counts, but I removed it because it wound up not actually working very well. This commit updates the spec to reflect that.

Anthony Super added 2 commits September 27, 2019 13:30

Add new element defs for 277s

2fad5f9

Properly parse 277s

e40ee1c

kputnam merged commit 72f4c29 into kputnam:master Jul 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Builder DSL, and add X212-HN277 Parser With It #200

Create Builder DSL, and add X212-HN277 Parser With It #200

AnthonySuper commented Jul 18, 2019

kputnam commented Jul 30, 2019

AnthonySuper commented Jul 31, 2019

AnthonySuper commented Sep 3, 2019

kputnam Sep 9, 2019 •

edited

Loading

AnthonySuper Sep 11, 2019

kputnam Sep 11, 2019

AnthonySuper Sep 12, 2019

kputnam Sep 9, 2019

AnthonySuper Sep 11, 2019

kputnam left a comment

kputnam commented Sep 9, 2019

AnthonySuper commented Sep 12, 2019

kputnam commented Sep 12, 2019

AnthonySuper commented Sep 13, 2019

kputnam commented Sep 13, 2019

kputnam commented Sep 13, 2019 •

edited

Loading

AnthonySuper commented Sep 16, 2019

kputnam commented Sep 16, 2019

AnthonySuper commented Oct 1, 2019

ram-nadella commented Jul 18, 2020

kputnam commented Jul 20, 2020

kputnam commented Jul 21, 2020

AnthonySuper commented Jul 21, 2020

Create Builder DSL, and add X212-HN277 Parser With It #200

Create Builder DSL, and add X212-HN277 Parser With It #200

Conversation

AnthonySuper commented Jul 18, 2019

Builder DSL

X212-HN277 Parsing

Final Thoughts

kputnam commented Jul 30, 2019

AnthonySuper commented Jul 31, 2019

AnthonySuper commented Sep 3, 2019

kputnam Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

AnthonySuper Sep 11, 2019

Choose a reason for hiding this comment

kputnam Sep 11, 2019

Choose a reason for hiding this comment

AnthonySuper Sep 12, 2019

Choose a reason for hiding this comment

kputnam Sep 9, 2019

Choose a reason for hiding this comment

AnthonySuper Sep 11, 2019

Choose a reason for hiding this comment

kputnam left a comment

Choose a reason for hiding this comment

kputnam commented Sep 9, 2019

AnthonySuper commented Sep 12, 2019

kputnam commented Sep 12, 2019

AnthonySuper commented Sep 13, 2019

kputnam commented Sep 13, 2019

kputnam commented Sep 13, 2019 • edited Loading

AnthonySuper commented Sep 16, 2019

kputnam commented Sep 16, 2019

AnthonySuper commented Oct 1, 2019

ram-nadella commented Jul 18, 2020

kputnam commented Jul 20, 2020

kputnam commented Jul 21, 2020

AnthonySuper commented Jul 21, 2020

kputnam Sep 9, 2019 •

edited

Loading

kputnam commented Sep 13, 2019 •

edited

Loading