Skip to content

Read FDF as JSON, promisified#41

Open
jpetso wants to merge 2 commits intojjwilly16:masterfrom
jpetso:master
Open

Read FDF as JSON, promisified#41
jpetso wants to merge 2 commits intojjwilly16:masterfrom
jpetso:master

Conversation

@jpetso
Copy link
Copy Markdown

@jpetso jpetso commented Jul 5, 2020

Hi, and thanks for this library!

I took @ocallarob's excellent changes from PR #30, rebased them on current master to resolve the conflict, squashed his three commits into one and made a few tweaks of my own:

  • His original static fdfToJSON() function isn't exposed by the module anymore. Instead, I introduced a new function readFdfAsJSON() that can be used instead of generateFdf().output() and resolves the promise with the JSON directly. This allows more succinct code and imho fits better with the pure streaming API exposed by the rest of the module. Although returning a non-Buffer value from the promise is a new concept for this library, so please have a look if this is what you want.
    • This pattern could be reused by functionality to also read JSON-ified dump_data_fields_utf8 output. Which is also easier to parse.
  • I improved the FDF parsing code - it now more strictly applies to just the section we're looking at, and is more robust (can deal with /T and /V being swapped, uses a longer / less common pattern for line splitting).
  • A brief example added to the README. I thought people might want to know about this.

Thanks to @ocallarob for the original changeset, hopefully this moves it in the right direction.

@jpetso
Copy link
Copy Markdown
Author

jpetso commented Jul 5, 2020

On second thought, maybe readFdfAsJSON() is too broad, given that it sometimes also contains other information? How about readFormFieldValuesAsJSON() (to leave room for readFormFieldInfoAsJSON() for JSON-ified dump_data_fields_utf8)?

@jpetso
Copy link
Copy Markdown
Author

jpetso commented Jul 5, 2020

Changed the function name to readFormFieldValuesAsJSON() as mentioned above. Also fixed eslint issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant