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

chore: added Pallas compatible comments #106

Merged
merged 1 commit into from
Sep 27, 2024
Merged

Conversation

tobehn
Copy link
Contributor

@tobehn tobehn commented Sep 4, 2024

I do not know if this is relevant to you.
When i saw Pallas i went through the system to see how it generates the HTML sites.

I realized it needs the comments in order to provide the small description for each function and struct.
Then i started going through Vib and added the descriptions in core everywhere it was missing.

If this is accepted i would go on with adding descriptions.
Let me know if this is wasted time.
Cheers.

Copy link
Member

@axtloss axtloss 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 actually very helpful, thanks for taking the time to create this

but I find the format of the comments to be a bit weird, the comments shouldn't include the function name, and also should be written in the present tense
as an example for the ChangeWorkingDirectory function:

// Add a WORKDIR instruction to the containerfile

@tobehn
Copy link
Contributor Author

tobehn commented Sep 4, 2024

Great to hear its helpful.

Well, i get your point.
I think i derived the comments from the ABRoot ones.

For example the comment for generateABGrubConf in ABRoot is //generateABGrubConf generates a new grub config with the given details > https://abroot.vanillaos.org/core.html#generateABGrubConf

Perhaps this is generally not the format style you'd prefer.
I think we should agree on a rather generic comment format for all !GO / Pallas documented software

@axtloss
Copy link
Member

axtloss commented Sep 4, 2024

Yeah I'm not a fan of the current commenting style, but until we have a set standard i'd keep the format i mentioned for vib

@tobehn
Copy link
Contributor Author

tobehn commented Sep 4, 2024

All right I am going to enhance the comments.

Copy link
Member

@axtloss axtloss left a comment

Choose a reason for hiding this comment

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

Already good, but one thing is that the return of a function should be seperarated
instead of "builds xyz and returns abc", it should be

builds xyz
returns: abc

core/compile.go Outdated Show resolved Hide resolved
core/finalize.go Outdated Show resolved Hide resolved
@mirkobrombin
Copy link
Member

Small note: Pallas does not care about how you define returns and parameters, it will threat both as a description and always parse both from the AST instead.

@axtloss
Copy link
Member

axtloss commented Sep 5, 2024

Small note: Pallas does not care about how you define returns and parameters, it will threat both as a description and always parse both from the AST instead.

Yeah I know, but it's easier to read from the description that way, since it makes it easier to spot

@tobehn
Copy link
Contributor Author

tobehn commented Sep 5, 2024

Small note: Pallas does not care about how you define returns and parameters, it will threat both as a description and always parse both from the AST instead.

This is what I also thought.
So in case I write the return into the comment it currently would be rendered into the Pallas generated description where you have a separate section for the returns below.

@mirkobrombin would it be possible to exclude everything from the comment starting with string “return:” from the description in Pallas?
Or even better use it as additional information for the Returns: section.

It then could fit the needs for reading in the code and in the dev documentation.

@mirkobrombin
Copy link
Member

It is possible, not sure if it is worth it, not sure if we should be opinionated in that side

@tobehn
Copy link
Contributor Author

tobehn commented Sep 5, 2024

It also came to my mind it might be difficult to track whether all returns of the function are recorded correctly in the comment.

I'd prefer to use the current auto generation of the functions returns and do not specifically create a section in the comment (=description). In some comments i could describe the returns though, in case they are important.

Copy link
Contributor Author

@tobehn tobehn left a comment

Choose a reason for hiding this comment

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

solved by ddd1e3a

core/compile.go Outdated Show resolved Hide resolved
core/finalize.go Outdated Show resolved Hide resolved
@axtloss
Copy link
Member

axtloss commented Sep 5, 2024

It is possible, not sure if it is worth it, not sure if we should be opinionated in that side

It is, pallas only tells the users the type of the return, not what it returns, which is very important to have

@axtloss
Copy link
Member

axtloss commented Sep 5, 2024

It also came to my mind it might be difficult to track whether all returns of the function are recorded correctly in the comment.

I don't see how it would be difficult, its not the exact value it returns, but what the return value means, if it's difficult to properly track what the function returns then the function needs to be reworked

cmd/build.go Outdated Show resolved Hide resolved
finalize-plugins/shell-final.go Show resolved Hide resolved
@mirkobrombin
Copy link
Member

mirkobrombin commented Sep 6, 2024

It is possible, not sure if it is worth it, not sure if we should be opinionated in that side

It is, pallas only tells the users the type of the return, not what it returns, which is very important to have

In that case a proper mapping would be better instead of just removing Returns from the comment

Or even better use it as additional information for the Returns: section.

Didn't see this comment

api/download.go Outdated Show resolved Hide resolved
api/finalize-scopes.go Outdated Show resolved Hide resolved
Copy link
Member

@axtloss axtloss left a comment

Choose a reason for hiding this comment

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

Some small nitpicks that i came across while going through the changes, starting to look pretty nice though

cmd/compile.go Outdated Show resolved Hide resolved
cmd/compile.go Outdated Show resolved Hide resolved
core/build.go Outdated Show resolved Hide resolved
core/build.go Outdated Show resolved Hide resolved
finalize-plugins/genimage.go Outdated Show resolved Hide resolved
finalize-plugins/shell-final.go Outdated Show resolved Hide resolved
finalize-plugins/shell-final.go Outdated Show resolved Hide resolved
@tobehn tobehn changed the title chore: added Pallas compatible comments to core chore: added Pallas compatible comments Sep 8, 2024
@axtloss
Copy link
Member

axtloss commented Sep 27, 2024

@tobehn any update on this? is it ready to merge?

@tobehn
Copy link
Contributor Author

tobehn commented Sep 27, 2024

@tobehn any update on this? is it ready to merge?

From my point of view the comments are done.

@axtloss
Copy link
Member

axtloss commented Sep 27, 2024

Alright, could you squash your commits into one so that I can merge it?

chore: improvement of Pallas compatible comments to core

chore: fixing line breaks

chore: improving comments based on review

chore: commented each and every function and struct in the project

fix typo

revert deletion of //export comments

added a few Returns: statements

fixes agreed on in review process

missing file in last commit
@tobehn
Copy link
Contributor Author

tobehn commented Sep 27, 2024

oh gosh, it looks to me like i messed it up.
I've never done a squashing before. Can you tell if i have done it right?

@axtloss
Copy link
Member

axtloss commented Sep 27, 2024

Yep, everything's great and how it's supposed to be.
Thanks for going through the effort and commenting everything!

@axtloss axtloss merged commit 3fb55c0 into Vanilla-OS:main Sep 27, 2024
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