Closed tetron closed 11 months ago
:warning: The sha of the head commit of this PR conflicts with #337. Mergify cannot evaluate rules on this PR. :warning:
@mr-c do we need to do anything else with this?
Righto, had a meeting with @mr-c just now , and believe we have an action plan, @tetron. Let me know what you think — feel free to chime in to add or correct anything, @mr-c.
CONTRIBUTING.md
and add pull request templates, to ensure we tell contributors what we expect in pull requests (this will simplify the promotion from main
to release
but can be done later, after we have the new release out). (moved to later here: https://github.com/common-workflow-language/user_guide/issues/391)I have the same permissions to this repository as you do, @tetron . During the meeting we have also confirmed @mr-c is not the sole person with admin/write actions (i.e. bus-factor > 1). Once we have verified the changes in this PR, and we are ready @mr-c will do a final review and merge it to release
.
Moving forward, I expect we will work out a better release cadence, having less issues with main
and being able to update the release
branch much faster due to the code contribution policies, and as the internships will have finished. I will volunteer more time to work on pull requests & issues in the user_guide
, and @mr-c will do the final review & merge (which is good as I am not a native speaker).
Cheers, -Bruno
First item from #337 that I checked was the issue #348, that was fixed on main
in pull request #358. main
contains a faq.md
where the entries are questions, as shown below seeing the faq.md
on GitHub UI.
Now the same file, from branch staging
:
I checked out the staging
branch locally, @tetron, and I get the same on my IDE, and after running make watch
:
I think something went wrong with the order of commits in this branch, @tetron ? EDIT: just realized that perhaps staging
is not bringing everything from main
into `release?
@swzCuroverse , @tetron , @mr-c , sorry for the delay. Vacation and holidays are now over, so I finally had time to i) go through every commit in #337, ii) opening the commit and linked PR, iii) confirming there are no pending feedback from @mr-c or others, and iv) updated this PR description with the ones that are still pending. Then, I v) went through the commits in this PR (#373) confirming we have the exact same commits as in #337, and vi) reviewed the difference in this PR (a few extra commits).
@tetron addressed some of the feedback in #337 in this PR (see commit "Apply suggestions made on https://github.com/common-workflow-language/user_guide/pull/337" https://github.com/common-workflow-language/user_guide/pull/373/commits/8bc1400208c390e18c872896fca4f5d4b4ac8a42)
But there are still a few items that are pending. I believe that if we either address those items as @tetron did in his commit to this PR, or if we reach a consensus that that can be moved to after the release, then this PR should finally be ready to be reviewed & merged by @mr-c or someone else from the project team :+1: (and that annoying checkbox in the user guide homepage will be finally fixed! :sweat_smile: ).
Again, sorry for the delay :bow:
p.s.: I'll attend a conference starting tomorrow early morning, with evening dinner/social activities, until Friday. So I may not respond very quickly; I left notes in the list above, and in some of the pending review feedback comments. I hope that helps :wave:
@kinow and @mr-c as requested in last-week's meeting, I've gone through the responses above. Please let me know your thoughts on my suggestions of moving forward:
PR Checklist Item | Discussion Links | Code Chunk: | Description | Status | Alexis' Comments |
---|---|---|---|---|---|
Network Access documentation�(commits not squashed,�discussion�about placement of the new text - might be pending a follow-up issue?) | https://github.com/common-workflow-language/user_guide/pull/335 | https://github.com/common-workflow-language/user_guide/pull/373/files#diff-2e1f5139279f1e0b9a18552a7a4df7cdc240e60443558cb9643d61b4b0650870R68-R81 | Network access chunk, commits not squashed and not sure if in the right section. | Pass | The positioning of this chunk / squashing of commits shouldn't hold up everything else, I'd just leave as it is for now, and a follow up PR can be made if a more appropriate place for this chunk can be made. Contribution guide has also been updated to prevent future issues with commits not being squashed. |
Vague comment in the FAQ��In CWL, everything must be directly stated� | https://github.com/common-workflow-language/user_guide/pull/337#discussion_r1009590758 | https://github.com/common-workflow-language/user_guide/pull/337#discussion_r1009590758 | There is a note stating `In CWL, everything must be directly stated | Action Required - revert | I think the best approach here would be to remove the note chunk entirely, it does not segue between the preceding and succeeding chunks at all and removing it would be at no loss to the reader |
Re-word sentence to make it easier to be read | https://github.com/common-workflow-language/user_guide/pull/337#discussion_r1010301829 | https://github.com/common-workflow-language/user_guide/pull/373/files#diff-488c38e152014417222886c68f51183169eeb1238139cb8125830f9b363b8070L161-R163 | Fair Principles have a quote that has been copied, OG PR wants to update to improve readability | Action Required - revert | I'd rather keep it 'as-is'. While I agree that the original wording could be improved, better to do by providing a better alternative to FAIR principles. |
Decide how to use quoted citations from other docs | https://github.com/common-workflow-language/user_guide/pull/337#discussion_r1010302708 | Same as above | How should we add quotes from other documents | N/A | I think this can be a discussion for a later date |
Use JavaScript instead of Js�in�using-containers.md | https://github.com/common-workflow-language/user_guide/pull/337#discussion_r1010306945 | https://github.com/common-workflow-language/user_guide/pull/373/files#diff-0be86c5faa5ab3b7b4889374ca862ea96b2924d7d03525aa441b42a3704efa17R44 | Change has been added in PR | Pass | Nothing to discuss |
Add "then" to avoid awkward pause�in�operations.md | https://github.com/common-workflow-language/user_guide/pull/337#discussion_r1010308465 | https://github.com/common-workflow-language/user_guide/pull/373/files#diff-e233cca7d068d7a4ad3e3c292da37078cda0681e8b52eeded14fd7319d5899c9R55 | Update wording from 'the' to 'then | Action Required - add suggestion | Add suggestion of 'then the command will fail since'� |
Decide on the placement of the Network Access section | https://github.com/common-workflow-language/user_guide/pull/337#discussion_r1010319610 | Same as item 1 | Whaere should the network access section go? | Pass | Decide later, leave for now to unblock this PR |
Thanks a lot @alexiswl !
@mr-c and others, if you want to look at the table on a big screen, here's what I executed on my browser console to bypass GitHub UI's max width: document.querySelector(".clearfix.new-discussion-timeline").style.setProperty('max-width', '100%')
:+1:
Agree on the pass. Comments on the non-pass entries:
Vague comment in the FAQ��In CWL, everything must be directly stated� I think the best approach here would be to remove the note chunk entirely, it does not segue between the preceding and succeeding chunks at all and removing it would be at no loss to the reader
:+1: “removing it would be at no loss to the reader” +1
Re-word sentence to make it easier to be read I'd rather keep it 'as-is'. While I agree that the original wording could be improved, better to do by providing a better alternative to FAIR principles.
Sounds good, easy to fix :+1:
Decide how to use quoted citations from other docs I think this can be a discussion for a later date
:+1:
Add "then" to avoid awkward pause�in�operations.md Add suggestion of 'then the command will fail since'�
Good suggestion. Easy to fix, not a blocker IMO (but we can still do that when merging this PR)
:clap:
:tada: :partying_face:
This supercedes #337
An interaction with branch protection on
main
means that theUpdate branch
andCommit suggestion
buttons don't work which is a problem for accepting minor text edits, and alsomain
is a moving target.I'm going to manually apply the suggestions made on #337, and then we can merge the
staging
branch to bothmain
andrelease
.Items blocking the merge / release of this PR
List of issues in this release (NOTE: it is fine to suggest moving issues to after a release, as long as that's agreed by both others, especially the OP of the comment :+1: ):
basic-concepts.md
pending reply or conclusion:using-containers.md
operations.md
command-line-tool.md
that were not addressed or are pending a resolutionnetworkdAccess: true
(explicit) instead of Network Access (this is pending in #337, but @tetron addressed it in this PR, in the commit in this PR 8bc1400208c390e18c872896fca4f5d4b4ac8a42 with commit message "Apply suggestions made on https://github.com/common-workflow-language/user_guide/pull/337")prerequisites.md
(this is pending in #337, but @tetron addressed it in this PR, in the commit in this PR 8bc1400208c390e18c872896fca4f5d4b4ac8a42 with commit message "Apply suggestions made on https://github.com/common-workflow-language/user_guide/pull/337"):For later / after the release
List of follow-up issues to be fixed after the new release (move items from the section above here if OP has agreed on doing so):
(we must list the issues above, and mark as checked only when there is nothing pending from feedback on a) this PR, b) on #337, and c) on the issue or PR fixed)