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

fix(parser): Unable to parse json in one line #43

Merged
merged 1 commit into from
Aug 27, 2024
Merged

fix(parser): Unable to parse json in one line #43

merged 1 commit into from
Aug 27, 2024

Conversation

iamxiaojianzheng
Copy link
Contributor

about this #41

cleaned_line = "'" .. cleaned_line

is_line_json_close(json_nesting_stack, opening_json_char, closing_json_char, cleaned_line)
Copy link
Owner

@oysandvik94 oysandvik94 Aug 27, 2024

Choose a reason for hiding this comment

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

This function returns a boolean whether the line contains a json closer, however the result is never used.
Is the case fixed if you do the same as in the else branch instead?

			local found_json_end =
				is_line_json_close(json_nesting_stack, opening_json_char, closing_json_char, cleaned_line)
			if found_json_end then
				cleaned_line = cleaned_line .. "'"
			end

If so, I think this method would be cleaner if the for loop looked like this:

	for _, line in ipairs(lines) do
		local cleaned_line = remove_trailing_forwardslash(line)

		if found_first_json_char(json_nesting_stack, cleaned_line) then
			opening_json_char = cleaned_line:sub(1, 1)
			closing_json_char = opening_json_char == "[" and "]" or "}"
		end
		local found_json_end =
			is_line_json_close(json_nesting_stack, opening_json_char, closing_json_char, cleaned_line)
		if found_json_end then
			cleaned_line = cleaned_line .. "'"
		end

		table.insert(cleaned_lines, cleaned_line)
	end

Copy link
Owner

@oysandvik94 oysandvik94 left a comment

Choose a reason for hiding this comment

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

See the comment.

Also, I think we should have a test for the case that fails.
It can be added to "parser_spec.lua", there are other examples in that file.

@iamxiaojianzheng
Copy link
Contributor Author

After reading the code, I found that although this method did not use its return value, it operated on json_nesting_stack, which was critical.

@iamxiaojianzheng
Copy link
Contributor Author

See the comment.

Also, I think we should have a test for the case that fails.

It can be added to "parser_spec.lua", there are other examples in that file.

OK, I'll add the test case later.

@oysandvik94
Copy link
Owner

After reading the code, I found that although this method did not use its return value, it operated on json_nesting_stack, which was critical.

That's right, might not be the best design from my part hehe.

But this check:
#json_nesting_stack == 0
Is done inside "is_line_json_close()", and the return value of that evaluation, so I think you can assert the result of that function instead of asserting the size of the stack afterwards. I think that will end up in more readable code.

@iamxiaojianzheng
Copy link
Contributor Author

iamxiaojianzheng commented Aug 27, 2024

I have finished the revision.

image

cleaned_line = "'" .. cleaned_line

-- if the all json content is in one line
Copy link
Owner

Choose a reason for hiding this comment

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

Dont need this comment, this if check will work for all cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. It's been taken care of.

@oysandvik94
Copy link
Owner

Looks very good! I just had a little nitpick

@oysandvik94 oysandvik94 merged commit 5087494 into oysandvik94:main Aug 27, 2024
2 checks passed
@oysandvik94
Copy link
Owner

Thank you so much for your contribution 😸

@iamxiaojianzheng
Copy link
Contributor Author

Thank you so much for your contribution 😸

Look forward to your refactoring. 😀

@iamxiaojianzheng iamxiaojianzheng deleted the fix/parse-json-error branch August 27, 2024 14:09
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.

2 participants