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

documentation field added to ContractDefinition-Node #2331

Merged
merged 4 commits into from
Jun 14, 2017

Conversation

djudjuu
Copy link
Contributor

@djudjuu djudjuu commented May 30, 2017

adresses #2243

@axic
Copy link
Member

axic commented May 30, 2017

Can you add a test case to ASTJSON which has a non-null documentation node?

@@ -92,7 +92,7 @@ BOOST_AUTO_TEST_CASE(basic_compilation)
BOOST_CHECK(dev::jsonCompactPrint(result["sources"]["fileA"]["AST"]) ==
"{\"attributes\":{\"absolutePath\":\"fileA\",\"exportedSymbols\":{\"A\":[1]}},"
"\"children\":[{\"attributes\":{\"baseContracts\":[null],\"contractDependencies\":[null],"
"\"contractKind\":\"contract\",\"fullyImplemented\":true,\"linearizedBaseContracts\":[1],"
"\"contractKind\":\"contract\",\"documentation\":null,\"fullyImplemented\":true,\"linearizedBaseContracts\":[1],"
Copy link
Member

Choose a reason for hiding this comment

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

The new indentation seems to follow the coding standard, but the line stands out. Maybe in a separate commit, you can fix the indentation around here.

@chriseth
Copy link
Contributor

chriseth commented Jun 6, 2017

@djudjuu please add a test that has a non-null documentation node, and also one that has a line break in the documentation.

@djudjuu
Copy link
Contributor Author

djudjuu commented Jun 6, 2017

@chriseth there already is one non-null documentation-test in ASTJSON.cpp. do you want me to add another one?

@djudjuu djudjuu force-pushed the ASTDocumentationEntry branch from 01f17da to ebdebc7 Compare June 6, 2017 13:48
@axic
Copy link
Member

axic commented Jun 7, 2017

@djudjuu would be nice to have a dedicated test in test/libsolidity/ASTJSON.cpp

Also can you add tests for non-legacy mode?

@axic
Copy link
Member

axic commented Jun 8, 2017

@djudjuu the dedicated was in reflection to @chriseth's comment. A test in test/ASTJSON for empty documentation.

@chriseth chriseth merged commit 21aafaa into develop Jun 14, 2017
@axic axic deleted the ASTDocumentationEntry branch June 14, 2017 12:34
@axic
Copy link
Member

axic commented Jun 14, 2017

@chriseth @djudjuu I think we should still add that test 😉

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.

5 participants