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

Duplicated comments of first table in an array of tables #131

Closed
TutorExilius opened this issue Sep 20, 2020 · 6 comments
Closed

Duplicated comments of first table in an array of tables #131

TutorExilius opened this issue Sep 20, 2020 · 6 comments

Comments

@TutorExilius
Copy link

I got problems with array of tables.

Comments of first table into the array will be duplicated.

Here, in case of an array of tables, each table will serialise its own comments. This is done by on of these lines:

toml11/toml/serializer.hpp

Lines 277 to 285 in c037913

if(!no_comment_)
{
for(const auto& c : item.comments())
{
token += '#';
token += c;
token += '\n';
}
}

toml11/toml/serializer.hpp

Lines 309 to 317 in c037913

if(!no_comment_)
{
for(const auto& c : item.comments())
{
token += '#';
token += c;
token += '\n';
}
}

But, after that happened, following lines will duplicate the comment of my first table:

toml11/toml/serializer.hpp

Lines 654 to 662 in c037913

if(!kv.second.comments().empty() && !no_comment_)
{
for(const auto& c : kv.second.comments())
{
token += '#';
token += c;
token += '\n';
}
}

Example:

Inputfile:

# comment 1
[[foo]]
# comment 2
[[foo]]
# comment 3
[[foo]]

Outputfile:

# comment 1
# comment 1
[[foo]]
# comment 2
[[foo]]
# comment 3
[[foo]]

(thanks slarti)

@TutorExilius TutorExilius changed the title Duplicated comments of multi-table Duplicated comments of array of table Sep 20, 2020
@TutorExilius TutorExilius changed the title Duplicated comments of array of table Duplicated comments of first table in an array of tables Sep 20, 2020
@ToruNiina
Copy link
Owner

Thank you for telling me the problem! I reproduced it. It is a bug. And the reason is as you pointed out.

A quick solution might be to check the first character of the tmp in the line 654. Checking types of the first element might fail to solve this in the case of heterogeneous arrays of which first element is an inline table.

The code for comments was added later than the serializer itself, so it's not very clean...
The best solution is refactoring, but it might take some time.

@TutorExilius
Copy link
Author

Thank you. I Hope you will be able to fix it soon :).
I was looking for a TOML Library and found that one, great job!

ToruNiina added a commit that referenced this issue Sep 22, 2020
@ToruNiina
Copy link
Owner

Although I will further refactor the code related to the comments, but I have added a first aid for this problem. I hope it will resolve the problem.

@voldemur
Copy link

voldemur commented Oct 9, 2020

@ToruNiina, Fix works only for empty first table. If table is not empty then comment is dublicated inside of the table.

input:

# comment 1
[[Test_table]]
abracadabra = 10

output:

# comment 1
Test_table = [
# comment 1
{abracadabra=10}
]

@ToruNiina
Copy link
Owner

Thank you for reporting it!

On the last saturday I found the core part of the problem reported here (still not fixed yet, though). Essentially, [[test_table]] defines an array that contains tables and a table that is the first element of the array at the same time. A comment on top of [[test_table]] is considered as a comment of both the array and the table element.

This is why both the array and the first element has the same comment in your example. Unfortunately, my patch only workarounds the case if [[]] format is selected. Because of the line width limit, your tables are inlined and my (ad-hoc) workaround does not work.

In order to fix this problem, we must first clarify whether the comment is for an array or an element. In my opinion, it is okay to consider the comment is for the first element in this case. Then I need to change the value initialization part, but this part is a bit tough and I'm now doing this.

Hopefully it will be fixed soon.

@ToruNiina
Copy link
Owner

I have just pushed the solution and tests. It seems to be fixed.

Thank you again for reporting the problem!

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

No branches or pull requests

3 participants