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

Fix: natspec annotations on constructors #3666

Closed
wants to merge 13 commits into from

Conversation

aarlt
Copy link
Member

@aarlt aarlt commented Mar 6, 2018

Part of #1141.

  • natspec annotations on constructore where ignored.

}
)";

char const *natspec = R"ABCDEF({
Copy link
Member

Choose a reason for hiding this comment

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

What is the ABCDEF doing here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It specifies a different delimiter. Otherwise, the string would end early in line 696 after uint256)"

Copy link
Member Author

Choose a reason for hiding this comment

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

ABCDEF is just a custom delimiter for the raw string literal.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right because the quote is right next to it: )"

@aarlt aarlt force-pushed the constructor_natspec branch from f80b555 to 6327805 Compare March 8, 2018 22:26
@aarlt
Copy link
Member Author

aarlt commented Apr 4, 2018

@chriseth @axic What is the current state here? Should I just recommit?

},
"return" : "The result of the multiplication and cookies with nutella"
},
"test(uint256,uint256)" : {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should use this name. The constructor does not really have a signature.

@axic what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I agree this could be improved.

I think there are a couple of different options:

  • not having a name, (uint256,uint256) - this should be fine since the fallback cannot have parameters (not preferred)
  • having an invalid character in the name, such as constructor:(uint256,uint256) (to reflect the current syntax)
  • keeping the contract name, since that is unambigious

Copy link
Member

Choose a reason for hiding this comment

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

Actually after #3923 it could just use consturctor(uin256,uin256) since constructor is a restricted name.

Then again, probably natspec needs to be redesigned at this point.

Copy link
Member Author

Choose a reason for hiding this comment

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

@axic ok, i would prefer constructor(..).. the questions is where to place that.. in methods, or should we do that within an extra json node...

{
"methods" : {
	"mul(uint256,uint256)" : {
		"notice" : "another multiplier"
	},
	"test(uint256,uint256)" : "this is a really nice constructor"
}
}

hmm.. interesting.. natspecs on fallbacks.. never tried that...

are there already ideas around "redesigning natspec"?

Copy link
Member

Choose a reason for hiding this comment

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

#1141 is the most comprehensive collection of issues

Copy link
Member

Choose a reason for hiding this comment

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

@aarlt can you please make that change or enable push access for us?

Copy link
Member Author

Choose a reason for hiding this comment

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

@axic @chriseth so the final version should be just constructor? I would prefer not to remove potentially useful information.. could imagine that it may be useful for some tools.. am i wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

however, i think next week i will have some time to checkout the natspec redesign.. anything else that should be done beside stuff mentioned in #1141 and #2243? @chriseth you wanted that arbitrary natspec tags are allowed, right? additionally to that I would like to be able to distinguish between natspec tags that need to preserve newlines.. or we just introduce a special json node that will hold the tag values with all the newlines.. it would also be nice to keep track of the line number - so it is possible to retrieve the location of a specific natspec tag within the solidity source.. @axic @chriseth what are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Can you create a new issue for that please?

Also I think we can just go forward with constructor in order to get this finally merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

@axic @chriseth Is it still correct to place constructor within the methods object?

{
	"methods" : 
	{
		"constructor" : "this is a really nice constructor"
	}
}

@aarlt aarlt force-pushed the constructor_natspec branch from 6327805 to cba2ce3 Compare July 21, 2018 14:44
@aarlt
Copy link
Member Author

aarlt commented Jul 21, 2018

hmm.. looks like that i did something wrong during the rebase.. will check later

- natspec annotations on constructore where ignored.
@aarlt aarlt force-pushed the constructor_natspec branch from cba2ce3 to 7997de4 Compare July 21, 2018 22:39
@aarlt aarlt closed this Jul 22, 2018
@aarlt aarlt deleted the constructor_natspec branch July 22, 2018 14:27
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.

4 participants