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

2023 January release #373

Merged
merged 56 commits into from
Nov 13, 2023
Merged

2023 January release #373

merged 56 commits into from
Nov 13, 2023

Conversation

tetron
Copy link
Member

@tetron tetron commented Jan 27, 2023

This supercedes #337

An interaction with branch protection on main means that the Update branch and Commit suggestion buttons don't work which is a problem for accepting minor text edits, and also main is a moving target.

I'm going to manually apply the suggestions made on #337, and then we can merge the staging branch to both main and release.

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 👍 ):

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)

dear-ore and others added 30 commits October 18, 2022 12:33
The command to install graphviz should be "sudo apt-get install graphviz" not "sudo apt get install graphviz".

Co-authored-by: Michael R. Crusoe <[email protected]>
`python3 -m venv venv` instead of `python -m venv venv`
* modified quick-start.md
* Update prerequisites.md
* Update basic-concepts.md
* Update faq.md
* Update yaml-guide.md

Co-authored-by: Peter Amstutz <[email protected]>
* fixed typos and punctuation errors

* Update best-practices.md

* Update best-practices.md

Co-authored-by: Peter Amstutz <[email protected]>
* docker clarity

* typo fix

Co-authored-by: Peter Amstutz <[email protected]>
corrected typos, corrected punctuations, rephrased  sentences, added a link where required

Co-authored-by: Peter Amstutz <[email protected]>
* Update basic-concepts.md

* Update command-line-tool.md
mr-c and others added 11 commits January 25, 2023 14:54
Currently translated at 3.4% (21 of 605 strings)

Translated using Weblate (Portuguese)

Currently translated at 2.1% (13 of 598 strings)

Translated using Weblate (Spanish)

Currently translated at 2.5% (15 of 598 strings)

Translated using Weblate (Spanish)

Currently translated at 100.0% (2 of 2 strings)

Translated using Weblate (Portuguese)

Currently translated at 100.0% (2 of 2 strings)

Co-authored-by: Michael Crusoe <[email protected]>
Translate-URL: https://hosted.weblate.org/projects/commonwl/cwl-user-guide-sphinx/es/
Translate-URL: https://hosted.weblate.org/projects/commonwl/cwl-user-guide-sphinx/pt/
Translate-URL: https://hosted.weblate.org/projects/commonwl/user-guide/es/
Translate-URL: https://hosted.weblate.org/projects/commonwl/user-guide/pt/
Translation: Common Workflow Language/CWL User Guide
Translation: Common Workflow Language/CWL User Guide: Sphinx
@tetron tetron mentioned this pull request Jan 27, 2023
@mergify

This comment was marked as outdated.

@tetron
Copy link
Member Author

tetron commented Feb 13, 2023

@mr-c do we need to do anything else with this?

@kinow
Copy link
Member

kinow commented Mar 6, 2023

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.

  • Review the changes in this pull request, comparing preview feedback in pull requests & issues, and from the other PR 2022 Week 44 release #337 confirming everything has been addressed (will comment with something like +1, -1, was fixed, was not fixed, could be better, etc.). (I will start going through this now.)
  • Fix anything that was not addressed correctly — if any.
  • Ignore the structure of git commits & merges in this PR, but,
  • update our 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: Improve our Contributing guide regarding squashing commits #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

@kinow

This comment was marked as outdated.

@kinow
Copy link
Member

kinow commented May 2, 2023

@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 #337" 8bc1400)

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 👍 (and that annoying checkbox in the user guide homepage will be finally fixed! 😅 ).

Again, sorry for the delay 🙇

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 👋

@alexiswl
Copy link
Contributor

@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?) #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� #337 (comment) #337 (comment) 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 #337 (comment) 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 #337 (comment) 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 #337 (comment) 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 #337 (comment) 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 #337 (comment) Same as item 1 Whaere should the network access section go? Pass Decide later, leave for now to unblock this PR

@kinow
Copy link
Member

kinow commented Oct 22, 2023

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%') 👍

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

👍 “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 👍

Decide how to use quoted citations from other docs
I think this can be a discussion for a later date

👍

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)

👏

alexiswl added a commit to alexiswl/cwl_user_guide that referenced this pull request Oct 25, 2023
As discussed here: common-workflow-language#373 (comment)

1. Basic Concepts - Roll back changes to quote fair principles verbatim

2. Operations - Reword the operation file sentence - this is contrary to the discussion on common-workflow-language#373 (comment) and instead have reworded this entirely - please discuss otherwise can go back to the 'then' addition.

3. FAQ - remove vague comment 'everything in CWL must be stated'

 Code 'note' is removed from the FAQ.md file
As discussed here: #373 (comment)

1. Basic Concepts - Roll back changes to quote fair principles verbatim

2. Operations - Reword the operation file sentence - this is contrary to the discussion on #373 (comment) and instead have reworded this entirely - please discuss otherwise can go back to the 'then' addition.

3. FAQ - remove vague comment 'everything in CWL must be stated'

 Code 'note' is removed from the FAQ.md file
@mr-c mr-c merged commit e4f63e0 into release Nov 13, 2023
1 check passed
@mr-c mr-c deleted the staging branch November 13, 2023 16:58
@kinow
Copy link
Member

kinow commented Nov 13, 2023

🎉 🥳

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.