Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Nov 21, 2024

The simple one-line-per-element format is recommended these days.

We have other tests that actually test both formats work (test_dash_s_list_parsing and test_dash_s_response_file_list)

The simple one-line-per-element format is recommended these days.
'onlylist_b_response': ([], True, '["main","__original_main","foo(int, double)","baz()","c_baz","Structy::funcy()"]'),
'onlylist_c_response': ([], False, '["main","__original_main","foo(int, double)","baz()","c_baz"]'),
'onlylist_b_response': ([], True, 'main\n__original_main\nfoo(int, double)\nbaz()\nc_baz\nStructy::funcy()\n'),
'onlylist_c_response': ([], False, 'main\n__original_main\nfoo(int, double)\nbaz()\nc_baz\n'),
Copy link
Member

Choose a reason for hiding this comment

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

In this case, isn't the original clearer actually? It avoids \nc and such, where the reader needs to think how to separate into tokens.

And isn't it good to keep testing for the older format, or do we have that testing elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See the PR description (I updated it). We have plenty of testing for old/new format.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed that. Then I still think this would be clearer the other way, but I don't feel strongly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case, isn't the original clearer actually? It avoids \nc and such, where the reader needs to think how to separate into tokens.

Its easier to read here in the python, but if you look at the test directory after the test runs and inspect the resp file, the one-element-per-line format is way easier to read.

I'd like to one day deprecate the old rsp json format.. (so not using outside of test_dash_s_list_parsing is a good first step)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its a little complex here because this python test file is trying to to encode the contetns of a file into a single-line-string in python, which will always look at little odd.

@sbc100 sbc100 enabled auto-merge (squash) November 21, 2024 01:03
@sbc100 sbc100 disabled auto-merge November 21, 2024 01:32
@sbc100 sbc100 merged commit 319b423 into emscripten-core:main Nov 21, 2024
23 of 28 checks passed
@sbc100 sbc100 deleted the rsp_files branch November 21, 2024 01:33
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