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

Toml header parsing fix #16141

Closed
wants to merge 4 commits into from

Conversation

ankushbhardwxj
Copy link
Member

@ankushbhardwxj
Copy link
Member Author

ankushbhardwxj commented Jul 27, 2020

@ben-albrecht Please review this & let me know for any changes.
Confirming testing passes on my end

> start_test test/mason test/library/packages/UnitTest test/library/packages/TOML
[Test Summary - 200728.003551]
[Summary: #Successes = 148 | #Failures = 0 | #Futures = 0 | #Warnings = 0 ]
[Summary: #Passing Suppressions = 0 | #Passing Futures = 0 ]
[END]

Copy link
Member

@ben-albrecht ben-albrecht left a comment

Choose a reason for hiding this comment

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

@ankingcodes - thanks for the quick turn-around. I have some testing feedback inline.

modules/packages/TOML.chpl Outdated Show resolved Hide resolved
test/library/packages/TOML/test/quoteTable.chpl Outdated Show resolved Hide resolved
test/library/packages/TOML/test/quoteTable.chpl Outdated Show resolved Hide resolved
@ankushbhardwxj
Copy link
Member Author

@ben-albrecht made changes requested.

@ankushbhardwxj ankushbhardwxj mentioned this pull request Jul 28, 2020
19 tasks
@ankushbhardwxj
Copy link
Member Author

ankushbhardwxj commented Jul 28, 2020

@ben-albrecht - Made the changes requested here. Also, modified TomlTest.chpl.
Confirming testing passes on my end:

[Test Summary - 200728.212300]
[Summary: #Successes = 23 | #Failures = 0 | #Futures = 0 | #Warnings = 0 ]
[Summary: #Passing Suppressions = 0 | #Passing Futures = 0 ]
[END]

We can also add a GSOC label here.

Copy link
Member

@ben-albrecht ben-albrecht left a comment

Choose a reason for hiding this comment

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

Passes local testing. I have 1 question about our implementation, but looks good otherwise!



// checks if quotes exist and avoids '.' within them to create subtables
proc splitWithoutQuotes(tblPath: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Will this do the right thing if we have mixed quotes, e.g.

[foo."a.b.c".bar]

If so, it could be useful to include this in the test cases.

Also, I suspect we don't yet handle escaped quotes, e.g.

[foo.\"a.b.c]

but I'm OK to defer that for future work.

Copy link
Member Author

Choose a reason for hiding this comment

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

[foo."a.b.c".bar]

No, our current implementation doesn't support this. We basically count the number of periods before a quote is encountered and split that many times. I'm not sure how we could handle this case, since we cannot differentiate periods before and after the quotes, using our inbuilt split function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe we will need to get rid of using the inbuilt split function and create a new function which would avoid periods inside of quotes.

Copy link
Member

Choose a reason for hiding this comment

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

I believe we will need to get rid of using the inbuilt split function and create a new function which would avoid periods inside of quotes.

I would support this approach. Let's discuss in today's call.

@ben-albrecht
Copy link
Member

Adding a WIP label to help me keep track of which PRs are waiting on me. IIRC, we are pursuing the parsing / splitting fixes in this PR.

@ben-albrecht
Copy link
Member

@ankingcodes - Should we close this PR until you have time to come back to it? I'm not sure if you'd pick up from this branch or start a new PR when you get around to the bigger parsing/splitting changes.

@ben-albrecht
Copy link
Member

@ankingcodes - I am going to close this PR until we have a chance to revisit the design & implementation.

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

Successfully merging this pull request may close these issues.

None yet

3 participants