Fix: natspec annotations on constructors by aarlt · Pull Request #4542 · argotorg/solidity

aarlt · 2018-07-22T14:43:42Z

natspec annotations on constructore where ignored.
as discussed: the constructor natspec will be placed within the methods node and will just have the name constructor.

Recreated PR. I recreated my constructor_natspec branch and cherry-picked the actual change. I wasn't aware of that this will also remove the original PR #3666, related to #1141.

codecov-io · 2018-07-24T23:28:09Z

Codecov Report

Merging #4542 into develop will increase coverage by 0.01%.
The diff coverage is 96.77%.

@@             Coverage Diff             @@
##           develop    #4542      +/-   ##
===========================================
+ Coverage    87.55%   87.57%   +0.01%     
===========================================
  Files          313      313              
  Lines        30810    30850      +40     
  Branches      3672     3665       -7     
===========================================
+ Hits         26977    27016      +39     
  Misses        2579     2579              
- Partials      1254     1255       +1

Flag	Coverage Δ
#all	`87.57% <96.77%> (+0.01%)`	⬆️
#syntax	`28.32% <27.41%> (?)`

axic · 2018-07-25T00:33:46Z

libsolidity/interface/Natspec.cpp

How about constructor = std::move(fillDevDocument(...) and fillDevDocumentation just return a Json::Value? That is a bit more consistent with the style we have.

Ok, I change the method to return a Json::Value. But I think that std::move is not needed here, because the compiler will already do return value optimization.

axic · 2018-07-25T00:35:42Z

libsolidity/interface/Natspec.cpp

I don't think return should be valid for the constructor.

Oh yeah! Good catch!

axic · 2018-07-25T13:47:37Z

libsolidity/analysis/DocStringAnalyser.cpp

Shouldn't this be constructor and not plural? What is the field used for?

Hmm.. I was also thinking about that, but then I saw that within DocStringAnalyser::handleCallable(..) functions is used, thats why I also used the plural here. It will be used to generate an error like Error: Doc tag @return not valid for constructors. - I had the feeling that plural is better readable.

Looks like that you want to have the singular here.

aarlt · 2018-08-02T18:27:09Z

@chfast hmm.. looks like that there is a problem with codecov, the current diff is not reflecting the actual changes.. or did i missed something?..

probably the lates analysis is just not yet finished.. was just wondering about that diff..

aarlt · 2018-08-02T18:30:55Z

I guess the stuff just changed because of squashing some commits. Let's see ;)

aarlt · 2018-08-02T21:47:01Z

@chfast for me it looks like an issue with codecov
@axic appveyor seem to hang.. probably because the initial commit was already done in march..

chfast · 2018-08-02T21:50:01Z

Codecov waits for all CI to complete, but when you force push commits to a PR it freak outs and can send out an invalid report. You will get a new one when AppVeyor finishes.

aarlt · 2018-08-02T21:50:56Z

@chfast thx! good to know

aarlt · 2018-08-03T00:38:40Z

@axic i saw accidentally that the natspecs are (now?) part of the meta-data. Regarding a potential change of the natspecs so that they may support arbitrary tags could have the advantage that users can add user-defined structured meta-data through those tags.

chriseth · 2018-08-06T15:35:02Z

libsolidity/interface/Natspec.cpp

style: { on a line of its own

axic · 2018-08-08T16:11:56Z

libsolidity/analysis/DocStringAnalyser.cpp

I think this indentation is not in line with the coding style.

axic · 2018-08-08T16:12:00Z

libsolidity/analysis/DocStringAnalyser.cpp

I think this indentation is not in line with the coding style.

axic · 2018-08-08T16:12:10Z

libsolidity/analysis/DocStringAnalyser.cpp

+		DocumentedAnnotation& _annotation
+)
+{
+	static const set<string> validTags = set<string>{"author", "dev", "notice", "param"};


Indentation fixed.

- natspec annotations on constructore where ignored.

aarlt · 2018-08-14T17:00:16Z

@axic @chriseth are there any problems with this PR?

chriseth · 2018-08-14T17:13:22Z

I'm sorry, @axic is travelling and I somehow missed your update.

feuGeneA · 2018-09-13T21:37:32Z

@chriseth Any idea when we can expect to see this in a release?

chriseth · 2018-09-13T21:43:46Z

It will be part of 0.5.0. Sorry for the inconvenience.

aarlt force-pushed the constructor_natspec branch 3 times, most recently from 0702fe3 to 28c421c Compare July 24, 2018 20:30

axic reviewed Jul 25, 2018

View reviewed changes

aarlt force-pushed the constructor_natspec branch 2 times, most recently from c4ec267 to 15a9eee Compare July 25, 2018 11:29

axic reviewed Jul 25, 2018

View reviewed changes

aarlt force-pushed the constructor_natspec branch 2 times, most recently from c3cc43d to 835e663 Compare August 2, 2018 18:20

chriseth reviewed Aug 6, 2018

View reviewed changes

aarlt force-pushed the constructor_natspec branch 2 times, most recently from 28a8dbf to d1673f3 Compare August 7, 2018 18:52

axic added the external contribution ⭐ label Aug 8, 2018

chriseth previously approved these changes Aug 8, 2018

View reviewed changes

axic reviewed Aug 8, 2018

View reviewed changes

libsolidity/analysis/DocStringAnalyser.cpp Outdated

Copy link

Contributor

axic Aug 8, 2018

Choose a reason for hiding this comment

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

I think this indentation is not in line with the coding style.

axic reviewed Aug 8, 2018

View reviewed changes

libsolidity/analysis/DocStringAnalyser.cpp Outdated

Copy link

Contributor

axic Aug 8, 2018

Choose a reason for hiding this comment

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

I think this indentation is not in line with the coding style.

axic reviewed Aug 8, 2018

View reviewed changes

axic self-assigned this Aug 8, 2018

aarlt dismissed chriseth’s stale review via 11b835b August 8, 2018 19:51

aarlt force-pushed the constructor_natspec branch from d1673f3 to 11b835b Compare August 8, 2018 19:51

Fix: natspec annotations on constructors

f76d4d5

- natspec annotations on constructore where ignored.

aarlt force-pushed the constructor_natspec branch from 11b835b to f76d4d5 Compare August 8, 2018 19:53

chriseth approved these changes Aug 14, 2018

View reviewed changes

chriseth merged commit 8f27fb1 into argotorg:develop Aug 14, 2018

feuGeneA mentioned this pull request Sep 13, 2018

[ethereum-types, react-docs, sol-compiler, sol-doc, types, typescript-typings, website] Add new Solidity documentation generation utility 0xProject/0x-monorepo#1004

Merged

16 tasks

feuGeneA mentioned this pull request Sep 18, 2018

Missing entries in devdoc / userdoc [natspec] #1141

Closed

6 tasks

Conversation

aarlt commented Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 24, 2018 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarlt Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarlt commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aarlt commented Aug 2, 2018

Uh oh!

aarlt commented Aug 2, 2018

Uh oh!

chfast commented Aug 2, 2018

Uh oh!

aarlt commented Aug 2, 2018

Uh oh!

aarlt commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarlt commented Aug 14, 2018

Uh oh!

chriseth commented Aug 14, 2018

Uh oh!

feuGeneA commented Sep 13, 2018

Uh oh!

chriseth commented Sep 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments

aarlt commented Jul 22, 2018 •

edited

Loading

codecov-io commented Jul 24, 2018 •

edited by codecov bot

Loading

aarlt Jul 25, 2018 •

edited

Loading

aarlt commented Aug 2, 2018 •

edited

Loading