The Wayback Machine - https://web.archive.org/web/20210701174033/https://github.com/ethereum/solidity/issues/11427
Skip to content

/ solidity

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

Jump to bottom

[isoltest] Support comments between expectations #11427

Open
axic opened this issue May 21, 2021 · 6 comments
Open

[isoltest] Support comments between expectations #11427

axic opened this issue May 21, 2021 · 6 comments
Labels
difficulty: medium good first issue testing 🔨
Projects
Solidity

Comments

@axic
Copy link
Member

@axic axic commented May 21, 2021

This would increase readability in bigger tests, such as those in semanticTests/externalContracts.

// -- Double reserve
// account: 1
// reserve(string), 69 ether: 0x20, 3, "abc" ->
// owner(string): 0x20, 3, "abc" -> 0x1212121212121212121212121212120000000012
// account: 0
// -- Transfer
// setContent(string,bytes32): 0x40, 0, 3, "abc" ->
// transfer(string,address): 0x40, 555, 3, "abc" ->
// owner(string): 0x20, 3, "abc" ->
// content(string): 0x20, 3, "abc" ->

The proposed syntax is -- text. Note that there is already # test # as a syntax which works at some places and not others. It would be fine making that work properly on new lines too.
@axic axic mentioned this issue May 21, 2021
Merged
@cameel
Copy link
Member

@cameel cameel commented May 21, 2021 •
edited

-- looks good to me. Something like # or ## would look more like a comment but stands out less:

// ## Double reserve
// account: 1
// reserve(string), 69 ether: 0x20, 3, "abc" ->
// owner(string): 0x20, 3, "abc" -> 0x1212121212121212121212121212120000000012
// account: 0
// ## Transfer
// setContent(string,bytes32): 0x40, 0, 3, "abc" ->
// transfer(string,address): 0x40, 555, 3, "abc" ->
// owner(string): 0x20, 3, "abc" ->
// content(string): 0x20, 3, "abc" ->

Something that would help even more would be ability to add empty lines:

// -- Double reserve
// account: 1
// reserve(string), 69 ether: 0x20, 3, "abc" ->
// owner(string): 0x20, 3, "abc" -> 0x1212121212121212121212121212120000000012
// account: 0
//
// -- Transfer
// setContent(string,bytes32): 0x40, 0, 3, "abc" ->
// transfer(string,address): 0x40, 555, 3, "abc" ->
// owner(string): 0x20, 3, "abc" ->
// content(string): 0x20, 3, "abc" ->
@cameel cameel added this to New issues in Solidity via automation May 21, 2021
@axic
Copy link
Member Author

@axic axic commented May 21, 2021

Yes, in my example branch (#11426) I used empty lines, but removed them since.
@cameel
Copy link
Member

@cameel cameel commented May 21, 2021

Having just reviewed it, I think it would have been easier with those empty lines :)
@axic
Copy link
Member Author

@axic axic commented May 21, 2021

My motivation exactly 😅 It is a pain dealing with that currently.
@dumbledorep
Copy link

@dumbledorep dumbledorep commented Jun 16, 2021

Cameel that's a good catch with the empty lines...the empty lines actually helps. It makes it easier and a lot more readable.
@axic
Copy link
Member Author

@axic axic commented Jun 16, 2021

Regarding ease of implementation, I think // @: "My comment" could be very easily supported with the hook system by @aarlt, but actual empty lines is a much bigger change in both the parser and printer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium good first issue testing 🔨
3 participants
@axic @cameel @dumbledorep