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

[DOC-349] Updated documentation including links to examples #156

Merged
merged 7 commits into from
Dec 19, 2023

Conversation

sitvanit83
Copy link
Contributor

@sitvanit83 sitvanit83 commented Oct 16, 2023

@sitvanit83 sitvanit83 requested a review from a team as a code owner October 16, 2023 05:40
Copy link
Contributor

@mdgeorge4153 mdgeorge4153 left a comment

Choose a reason for hiding this comment

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

Some comments. Please also check that the CI is passing (looks like there is a typo), and add the link to the generated documentation to the PR comment (see the instructions in the default PR comment template).

docs/cvl/defs.md Outdated Show resolved Hide resolved
docs/cvl/functions.md Outdated Show resolved Hide resolved
docs/cvl/ghosts.md Outdated Show resolved Hide resolved
docs/cvl/ghosts.md Outdated Show resolved Hide resolved
docs/cvl/defs.md Outdated Show resolved Hide resolved
docs/cvl/methods.md Outdated Show resolved Hide resolved
docs/cvl/statements.md Outdated Show resolved Hide resolved
docs/cvl/statements.md Outdated Show resolved Hide resolved
docs/cvl/using.md Outdated Show resolved Hide resolved
docs/cvl/statements.md Outdated Show resolved Hide resolved
@mdgeorge4153 mdgeorge4153 changed the title Updated documentation including links to examples [DOC-349] Updated documentation including links to examples Oct 17, 2023
Copy link
Contributor

@mdgeorge4153 mdgeorge4153 left a comment

Choose a reason for hiding this comment

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

Another round of comments.

I think the biggest thing is that some of the examples need some context - see my comment on defs.md for an example of what I mean.

For the examples that we expect to describe fully in the user guide (like the ERC20 examples linked to for rules, assert, and require statements), I wonder if it might be better to put a placeholder outline in the "Spec by Example" section of the user guide and move the example links there. Maybe it should be a separate PR though.

docs/cvl/functions.md Outdated Show resolved Hide resolved
docs/cvl/functions.md Outdated Show resolved Hide resolved
docs/cvl/functions.md Outdated Show resolved Hide resolved
docs/cvl/functions.md Outdated Show resolved Hide resolved
docs/cvl/functions.md Outdated Show resolved Hide resolved
docs/user-guide/tutorials.md Outdated Show resolved Hide resolved
docs/cvl/hooks.md Outdated Show resolved Hide resolved
docs/cvl/imports.md Outdated Show resolved Hide resolved
docs/cvl/imports.md Outdated Show resolved Hide resolved
docs/cvl/imports.md Outdated Show resolved Hide resolved
Copy link
Contributor

@nd-certora nd-certora left a comment

Choose a reason for hiding this comment

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

overall good, a few comments and requests to do a bit more cleaning

docs/cvl/defs.md Outdated Show resolved Hide resolved
docs/cvl/functions.md Outdated Show resolved Hide resolved
docs/cvl/functions.md Outdated Show resolved Hide resolved
docs/cvl/ghosts.md Outdated Show resolved Hide resolved
docs/cvl/ghosts.md Outdated Show resolved Hide resolved
docs/cvl/statements.md Outdated Show resolved Hide resolved
docs/cvl/statements.md Show resolved Hide resolved
docs/cvl/statements.md Outdated Show resolved Hide resolved
docs/cvl/using.md Show resolved Hide resolved
docs/cvl/using.md Outdated Show resolved Hide resolved
@sitvanit83 sitvanit83 requested a review from nd-certora October 31, 2023 14:24
Copy link
Contributor

@mdgeorge4153 mdgeorge4153 left a comment

Choose a reason for hiding this comment

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

Looks like a bunch of the links got broken so I can't view the generated documentation. Can you fix them and re-request review?

Thanks

docs/cvl/defs.md Outdated Show resolved Hide resolved
docs/cvl/statements.md Outdated Show resolved Hide resolved
docs/cvl/statements.md Outdated Show resolved Hide resolved
docs/cvl/ghosts.md Show resolved Hide resolved
docs/cvl/ghosts.md Outdated Show resolved Hide resolved
@sitvanit83 sitvanit83 requested a review from nd-certora November 5, 2023 14:16
Copy link
Contributor

@mdgeorge4153 mdgeorge4153 left a comment

Choose a reason for hiding this comment

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

Overall looking good, here's another round of edits.

docs/cvl/defs.md Outdated Show resolved Hide resolved
docs/cvl/defs.md Outdated Show resolved Hide resolved
docs/cvl/expr.md Outdated Show resolved Hide resolved
docs/cvl/expr.md Outdated Show resolved Hide resolved
docs/cvl/expr.md Outdated Show resolved Hide resolved
docs/cvl/using.md Show resolved Hide resolved
docs/cvl/using.md Outdated Show resolved Hide resolved
docs/cvl/using.md Outdated Show resolved Hide resolved
docs/cvl/using.md Outdated Show resolved Hide resolved
docs/user-guide/tutorials.md Outdated Show resolved Hide resolved
@nd-certora nd-certora merged commit 0f05376 into master Dec 19, 2023
1 check passed
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.

3 participants