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

Package-level documentation (#3281) #3785

Merged
merged 4 commits into from
Jul 27, 2021
Merged

Package-level documentation (#3281) #3785

merged 4 commits into from
Jul 27, 2021

Conversation

rhagenson
Copy link
Member

Not done yet, but contains the initial pass adding to the style guide and changing the stdlib to confirm to the style addition.

Throughout this process I ran into a few "I wish that was better" instances such as those listed in the issue itself. I am in favor of the <PACKAGE>.pony matching in name exactly -- so not added/missing underscores or "s" at the end. However, with the name possibly being confusing (see promise.pony versus promises.pony) I am in favor of making the style to add to some package.pony or other common name rather than copy the package name itself.

Thoughts on the above?

An initial review is fitting, but I expect additional needs to be met before merging. (Some of the package-level documentation I added is meager, but I did not have much to go off of for say strings when stating the purpose of the package.)

@rhagenson
Copy link
Member Author

As I work on this, is there a way to only run the linter and spellcheck or prevent testing/cross-compilint the stdlib on all platforms?

@SeanTAllen
Copy link
Member

@rhagenson no. All tests run. There's no way to pick and choose unless you remove them from the configuration files.

@rhagenson
Copy link
Member Author

Seems an awful waste of CI resources, but the alternative is being able to just turn off failing checks so makes sense. Thank you.

@EpicEric EpicEric added the do not merge This PR should not be merged at this time label Jul 8, 2021
@rhagenson rhagenson changed the title Initial progress towards #3281 Package-level documentation (#3281) Jul 8, 2021
Copy link
Contributor

@Theodus Theodus left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Just some minor suggestions

packages/collections/collections.pony Outdated Show resolved Hide resolved
packages/strings/strings.pony Outdated Show resolved Hide resolved
packages/term/term.pony Outdated Show resolved Hide resolved
@ergl
Copy link
Member

ergl commented Jul 13, 2021

A couple of issues that might be related and could be closed when this is merged: #725 and #616

rhagenson and others added 3 commits July 13, 2021 19:46
Co-authored-by: Theo Butler <theodusbutler@gmail.com>
Co-authored-by: Theo Butler <theodusbutler@gmail.com>
Co-authored-by: Theo Butler <theodusbutler@gmail.com>
@rhagenson
Copy link
Member Author

@ergl I agree that those issues could be rolled into this PR.

For #725, it seems to be done? If the additional work needed is in regards to encoding, then I think #3679 working on rfcs#179 hitting main will require continued work on #725. correct?

For #616, I have not used INI before so whereas I could read the Wikipedia article and take a best shot at improving the docs, I am going into it without prior knowledge.

@rhagenson
Copy link
Member Author

@Theodus You just majorly flexed on me with those suggested changes. I had no idea that it was possible to have the changes appear with a "commit changes" prompt. I have made similar typo and wording suggestions on the Tutorial. I could have made life so much easier on myself and others if I knew that GitHub had such a feature. Thank you for showing me that was possible.

@Theodus
Copy link
Contributor

Theodus commented Jul 14, 2021

@rhagenson no problem! I had the same reaction when I first saw this feature used.

@SeanTAllen
Copy link
Member

@rhagenson is this ready? it looks ready to those of use looking at this during the sync call.

@rhagenson
Copy link
Member Author

If it looks ready to y'all then it probably is ready.

The lasting concern was: Do we want a different style to what/how to name the package level documentation to avoid the situations Andy brought up in the original issue?

@SeanTAllen
Copy link
Member

@rhagenson what issue brought up by andy?

@rhagenson
Copy link
Member Author

In #3281 Andy mentioned a couple instances of "is this close enough" and suggested the format I went with, which was using a file named the same as the package itself. By no means a blocking concern, but if we wanted to have a more universal standard such as all package level documentation is in package.pony rather than say files.pony for the files package, I would make those changes (and Style Guide change) before this is merged.

@SeanTAllen
Copy link
Member

@rhagenson i dont think the close enough is close enough. i think it should match the style guide.

@rhagenson
Copy link
Member Author

Agreed. My point is then: do we want to change the style guide to avoid instances where it would make sense to have it be "close enough"?

If y'all are okay with this PR as it stands and we do not want to change the style guide further then this PR is ready to merge.

@SeanTAllen
Copy link
Member

SeanTAllen commented Jul 27, 2021

@rhagenson i dont understand what we would change about the style guide. it's very clear as to naming. i think the "close enough" files that andy mentions are style guide violations and should be fixed.

also i'm 98% sure i created those original violations. doh. in my defense, i dont think there was a style guide then.

@rhagenson
Copy link
Member Author

I think part of the confusion here may be that I added the "Package-level Documentation" subsection as part of this PR. According to that addition, the "close enough" instances are violations, but since adding to the style guide is part of this PR we could change if/how/why they are violations.

At think at this point though any conversation around how we want to handle naming the package level documentation file is delaying without benefit.

@SeanTAllen
Copy link
Member

I think the style guide is good with re to the changes about package level docs that are in this PR @rhagenson

@rhagenson
Copy link
Member Author

Got it. Then if the decision during sync was all good, this is ready to merge.

@SeanTAllen SeanTAllen merged commit 829b5c0 into main Jul 27, 2021
@SeanTAllen SeanTAllen deleted the issue-3281 branch July 27, 2021 20:44
@EpicEric EpicEric removed the do not merge This PR should not be merged at this time label Jul 28, 2021
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.

6 participants