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

Natspec completions #359

Merged
merged 9 commits into from
Jan 20, 2023
Merged

Natspec completions #359

merged 9 commits into from
Jan 20, 2023

Conversation

antico5
Copy link
Collaborator

@antico5 antico5 commented Jan 4, 2023

closes #342
closes #343
closes #296

This one is branched off a rebased version of PR #328

Copy link
Member

@kanej kanej left a comment

Choose a reason for hiding this comment

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

This is great.

I have pinged the channel to confirm people are happy with the suggestion we are making to users.

Reading through the spec I suspect we need to support completions on /// comments as well. Does that require a different approach?

server/src/services/completion/natspec.ts Outdated Show resolved Hide resolved
@antico5
Copy link
Collaborator Author

antico5 commented Jan 5, 2023

This is great.

I have pinged the channel to confirm people are happy with the suggestion we are making to users.

Reading through the spec I suspect we need to support completions on /// comments as well. Does that require a different approach?

I think we can achieve that with some small adaptation. Should I add that to this PR?

@kanej
Copy link
Member

kanej commented Jan 5, 2023

This is great.
I have pinged the channel to confirm people are happy with the suggestion we are making to users.
Reading through the spec I suspect we need to support completions on /// comments as well. Does that require a different approach?

I think we can achieve that with some small adaptation. Should I add that to this PR?

Yes, lets add that to the requirement.

@antico5
Copy link
Collaborator Author

antico5 commented Jan 17, 2023

@kanej triple forward slash format is implemented! ready to re-review

@antico5 antico5 changed the title Natspec completions: contracts, interfaces, libraries, state variables and events Natspec completions Jan 18, 2023
@kanej
Copy link
Member

kanej commented Jan 19, 2023

Based on feedback can we change:

function

  • remove @dev and @notice and rely on the default text at the top to populate @notice
  • do not put in @return if there is only one return entry

Contract/Library/Interface

Event

State Variable

Public state variable

Can we check that the gif matches this version?

@antico5
Copy link
Collaborator Author

antico5 commented Jan 19, 2023

@kanej ready for review!

Copy link
Member

@kanej kanej left a comment

Choose a reason for hiding this comment

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

I have tested and generated docs from the defaults using solc: https://docs.soliditylang.org/en/latest/natspec-format.html#documentation-output.

I am approving, but can I request one change which is that public state variables should not default to @dev, just @notice.

@antico5 antico5 merged commit 38f4d3b into development Jan 20, 2023
@antico5 antico5 deleted the natspec_full branch January 20, 2023 14:55
antico5 added a commit that referenced this pull request Jan 23, 2023
* feat: automatic natspec documentation

* tests for natspec completion

* add natspec completion gif to readme

* add tab-jumping and return param name

* feat: natspec completion for contracts, interfaces and libraries

* feat: natspec completion for state vars and events

* feat: natspec triple slash completion

* fix: update logic on natspec generation

* fix: show either @notice or @dev on state variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
2 participants