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

Add a documentation section for Concepts/Finalize #285

Merged
merged 7 commits into from
Oct 4, 2023

Conversation

kpandl
Copy link
Contributor

@kpandl kpandl commented Sep 28, 2023

Closes #283

Copy link
Contributor

@collinc97 collinc97 left a comment

Choose a reason for hiding this comment

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

LGTM

image

@collinc97
Copy link
Contributor

Going to wait for @apruden2008 and @craigjson 's reviews before merging to production and closing the issue.

@craigjson
Copy link
Contributor

craigjson commented Oct 2, 2023

I see some repeated information in the existing Language/Functions section with this one.

Does it make sense to move the entire Functions section into a new page and document all of them in more detail, we can link to this new section in the existing portion in the Language section.

@collinc97
Copy link
Contributor

I see some repeated information in the existing Language/Functions section with this one.

Does it make sense to move the entire Functions section into a new page and document all of them in more detail, we can link to this new section in the existing portion in the Language section.

Is the repeated information inconsistent?

How large of a change to the existing language guide would be needed to modify the existing docs @craigjson ?

Can you give @kpandl a checklist of line numbers and paragraphs to address the feedback?

@kpandl
Copy link
Contributor Author

kpandl commented Oct 3, 2023

Thank you for the feedback, @craigjson and @collinc97.

The finalize file contains more detailed information on finalize than the language file. So all of the information of the language file is also in the finalize file, but not necessarily the other way. From my understanding, the repeated information is consistent.

Speaking of line numbers, the following line numbers in the finalize file add information over the language file:

  • Line 7 (used to run computations on chain, primary purpose of public on chain state/mappings initiation/change)
  • Line 14 (Aleo network nodes, only code in finalize updates mappings, only program can write into its own mapping, Aleo network nodes can read public state)
  • Line 16 (explanation of the example code)
  • Line 52 (explanation when finalize may be unnecessary)

Speaking of line numbers in the language file, information from all lines on finalize (366-413) is also contained in the finalize file.

Going forward, one way to address it would be by making the information in the finalize file more dense, for example by removing the code from lines 377-413 (as the code is also in the finalize section and explained in more depth) and mention in the sentence with the link (line 376) that there is a code example. However, the description of finalize in the language section may then be less consistent with the description of other functions which have an example. I don't think we should remove any information on finalize from lines 368-374.
Alternatively, we could include all of the information of the finalize file in the language file, if the standalone finalize file is not necessary to have (I think this is my preferred option right now).
@craigjson yes another alternative would be to have a separate functions section if you see room for more detailed descriptions on the other functions.

What do you think? Happy to implement the changes

Copy link
Contributor

@craigjson craigjson left a comment

Choose a reason for hiding this comment

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

I don't see an issue with the Finalize file being a super-set of the language/finalize portion.

The thing I noticed was that from a structure POV, the standalone Finalize section is lower level than any of the other sub-topics under Leo.

Alternatively, we could include all of the information of the finalize file in the language file, if the standalone finalize file is not necessary to have (I think this is my preferred option right now).

This is my favorite solution also.

If finalize is the only function that is getting more detailed information, keeping things consistent and in one place would be my preference.

documentation/leo/10_finalize.md Outdated Show resolved Hide resolved
@kpandl kpandl requested a review from collinc97 October 4, 2023 12:35
Copy link
Contributor

@collinc97 collinc97 left a comment

Choose a reason for hiding this comment

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

LGTM

@collinc97 collinc97 merged commit 5a9b70c into master Oct 4, 2023
2 checks passed
@collinc97 collinc97 deleted the kpandl/finalize-1 branch October 4, 2023 18:25
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.

Update documentation to describe finalize scope, program mappings, and public state
3 participants