search

samba-in-kubernetes / samba-operator

An operator for a Samba as a service on PVCs in kubernetes
Apache License 2.0
108 stars 24 forks source link

Add comments to some packages #239

Closed phlogistonjohn closed 2 years ago

phlogistonjohn commented 2 years ago

This will fix errors detected by revive as part of running make check. It's genuinely a good thing to have docs, but in some cases they could use a little fleshing out.

anoopcs9 commented 2 years ago

I see that in some places package comments were added via new doc.go and for others existing files were used. Is there a particular reason to do so?

synarete commented 2 years ago

I see that in some places package comments were added via new doc.go and for others existing files were used. Is there a particular reason to do so?

As far as I can tell, not all places have doc.go. Thus, if doc.go exists, place the package comment there; otherwise, use another major file.

anoopcs9 commented 2 years ago

I see that in some places package comments were added via new doc.go and for others existing files were used. Is there a particular reason to do so?

As far as I can tell, not all places have doc.go. Thus, if doc.go exists, place the package comment there; otherwise, use another major file.

Yes, but the patch adds new doc.go files under internal/planner, internal/resources, internal/smbcc and tests/utils/kube whereas comments could have been included within existing files under those directories/packages.

Checking again now I think the criteria is more like minimum number of source files present under a particular directory/package(>3) qualifying for an extra doc.go 😆. But then package smbcc should not have one 🤨

phlogistonjohn commented 2 years ago

I see that in some places package comments were added via new doc.go and for others existing files were used. Is there a particular reason to do so?

Yeah, if the the package was a single file and I expect it to stay that way for the near term I just added a comment to the existing file. If the package had many files I added a doc.go (doc.go is a common convention in Go).

phlogistonjohn commented 2 years ago

Checking again now I think the criteria is more like minimum number of source files present under a particular directory/package(>3) qualifying for an extra doc.go laughing. But then package smbcc should not have one raised_eyebrow

I might break that one up in the future.

phlogistonjohn commented 2 years ago

I'm thinking this PR is ok to merge now. Since it's got one approval my plan is to merge it tomorrow unless I see some explicit NACKs. Thanks.