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.

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

Changes for broken struct issue with text parser #684

Merged
merged 5 commits into from Mar 16, 2021

Conversation

desaikd
Copy link
Contributor

@desaikd desaikd commented Mar 15, 2021

Issue #682

Description of changes:
Changes for throwing error on broken struct as input for text parser.

Input:

{broken

Output before change:

Struct { _fields: [Object: null prototype] {} }  // null struct

Output after change:

Error: expected ':'  // throws errror

Test:
added unit tests for text reader and dom load

@desaikd desaikd requested a review from zslayton March 15, 2021 17:21
Copy link
Contributor

@zslayton zslayton left a comment

Choose a reason for hiding this comment

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

Good find! Could you add a unit test for this?


let ionReader = ion.makeReader(invalidIonToRead);
ionReader.next();

Choose a reason for hiding this comment

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

why wouldn't you use const instead of let since you don't modify or change the value of invalidIonToRead and ionReader

Copy link
Contributor

Choose a reason for hiding this comment

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

Upgrading let to const where appropriate is something we intend to do project-wide via one of our linting tools but haven't had the opportunity yet. Once we do, new code will be required to const where possible.

src/IonParserTextRaw.ts Show resolved Hide resolved
@desaikd desaikd merged commit 82b940c into amazon-ion:master Mar 16, 2021
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.

None yet

3 participants