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

Added or expanded 5 comments… #9340

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

liamzee
Copy link
Collaborator

@liamzee liamzee commented Oct 15, 2023

…I documented lightly defaultMainHelper in Cabal/Distribution.Simple, expandResponse in Cabal/Distribution.Compat.ResponseFile, expanding the main comment in cabal-install/Distribution.Client.Main, adding segmentation commenting marking responseFiles handling in cabal-install/Distribution.Client.Main (main), added haddock documentation to mainWorker in cabal-install/Distribution.Client.Main .

@liamzee liamzee added the attention: needs-help Help wanted with this issue/PR label Oct 15, 2023
@ffaf1
Copy link
Collaborator

ffaf1 commented Oct 15, 2023

@liamzee , I see closed PRs with similar name, remember you can open one, wait for CI to finish and then update it (push with --force).

Is fourmolu pestering you? Check this bit of CONTRIBUTING.md.

If you need more help, just ask!

@liamzee liamzee force-pushed the comment-branch-really-fixed branch 4 times, most recently from ab51769 to 8ba1b71 Compare October 15, 2023 12:09
@liamzee
Copy link
Collaborator Author

liamzee commented Oct 15, 2023

I know the implicit issue, i.e, some maintainers have e-mail alerts set to any attempt at PR, and I just spammed multiple people's inboxes, and I'm really sorry about that.

make style is apparently going berserk, but I'm not going to ask for people's time when I can't even get my fourmolu working on VSC (It's apparently either ignoring the fourmolu.yaml or just diverting to a different piece of software).

Hope that something at least useful comes out of this PR.

@liamzee liamzee changed the title Added or expanded 5 comments, and fourmolu-formatted affected files. … Added or expanded 5 comments… Oct 15, 2023
@ffaf1
Copy link
Collaborator

ffaf1 commented Oct 15, 2023

I know the implicit issue, i.e, some maintainers have e-mail alerts set to any attempt at PR, and I just spammed multiple people's inboxes, and I'm really sorry about that.

Contributions are always appreciated, helping each other is paramount!

fourmolu CI check passed, well done.

@liamzee liamzee removed the attention: needs-help Help wanted with this issue/PR label Oct 15, 2023
@ffaf1
Copy link
Collaborator

ffaf1 commented Oct 15, 2023

Commit message should be formatted to standard way, as per CONTRIBUTING.md. Something like

Improve documentation

Add/expalnd comments for functions `defaultMainHelper`, `expandResponse`,
`responseFiles`, `mainWorker`, and for  `Distribution.Client.Main` module.

will do.

@liamzee
Copy link
Collaborator Author

liamzee commented Oct 15, 2023

What would you recommend as a path of action, then?

Step 1. Edit the commit message to match requirements; i.e, split the subject line with a summary,
then specify what occurred.

Step 2. Expand some of the comments to match line width expectations for comments.

Step 3. Be clearer on what a splitter is, or rather, remove the term entirely. Instead,
specify that it is the chooser for Cabal's simple build system to take an action.

Step 4. I am personally dissatisfied with one comment (mainHelper), as I don't think it's clear enough
on what one of the actions does. I was originally planning to revise the comment in a further
commit, but if I must revise, then it makes sense to make such changes immediately.

@ffaf1
Copy link
Collaborator

ffaf1 commented Oct 15, 2023

For sure, @liamzee!

@liamzee liamzee marked this pull request as draft October 15, 2023 16:07
@liamzee liamzee marked this pull request as ready for review October 15, 2023 23:30
Copy link
Collaborator

@ffaf1 ffaf1 left a comment

Choose a reason for hiding this comment

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

Very good!

@ffaf1
Copy link
Collaborator

ffaf1 commented Oct 17, 2023

Add a merge me label and wait patiently for two days to have this merged, @liamzee.

@liamzee liamzee added the merge me Tell Mergify Bot to merge label Oct 17, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Oct 19, 2023
To help make the Cabal codebase more accessible,
expandResponse in Cabal/Distribution.ResponseFile
received Haddock documentation.

The defaultMainHelper function in Cabal/Distribution.Simple
received hidden Haddock documentation.

In the hidden module cabal-install/Distribution.Client.Main,
the Haddock documentation for main was expanded, additional
commenting explaining the response file compatibility
code in main was added, and documentation for
mainWorker was added.
@mergify mergify bot merged commit af02e57 into haskell:master Oct 19, 2023
44 checks passed
@liamzee liamzee deleted the comment-branch-really-fixed branch October 19, 2023 19:37
@liamzee liamzee restored the comment-branch-really-fixed branch October 19, 2023 21:18
@liamzee liamzee deleted the comment-branch-really-fixed branch October 20, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-review documentation merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants