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

feat: prompt messages #441

Merged
merged 6 commits into from
Jul 15, 2024
Merged

feat: prompt messages #441

merged 6 commits into from
Jul 15, 2024

Conversation

CodeWithEmad
Copy link
Member

Merge checklist:

Check off if complete or not applicable:

  • Changelog record added
  • Documentation updated (not only docstrings)
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, deadlines, tickets

This pull request adds "Prompt" messages to all cookie cutters, improving the template creation experience. Additionally, some cleanup was performed.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 26, 2024
@openedx-webhooks
Copy link

Thanks for the pull request, @CodeWithEmad! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@CodeWithEmad CodeWithEmad changed the title Feat/prompt feat: prompt messages Feb 26, 2024
@itsjeyd
Copy link

itsjeyd commented Mar 8, 2024

Hey @CodeWithEmad, thank you for this contribution! I'll line them up for a test run now.

@itsjeyd itsjeyd added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 8, 2024
@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 14, 2024
@itsjeyd
Copy link

itsjeyd commented Apr 12, 2024

Hey @CodeWithEmad, looks like you got some failing tests here. Let us know when you've had a chance to address those.

@CodeWithEmad
Copy link
Member Author

Sure @itsjeyd. My changes were pretty harmless :) I'll look into it.

@CodeWithEmad
Copy link
Member Author

CodeWithEmad commented Apr 16, 2024

@itsjeyd Tests are failing due to missing packages, possibly caused by Dependabot (also see #448). Fixed in #451. Probably mine will be fixed too after merging 451.

@itsjeyd
Copy link

itsjeyd commented Apr 22, 2024

Thanks @CodeWithEmad. It looks like #451 was closed as obsolete a few hours ago. Which might mean that some other PR containing relevant changes has landed on master by now (?). If so, the next step here would probably be to rebase and see if that gets the build to pass :)

@itsjeyd
Copy link

itsjeyd commented May 23, 2024

Hey @CodeWithEmad, just checking in to see if you're still planning to continue working on this PR?

@CodeWithEmad
Copy link
Member Author

CodeWithEmad commented May 25, 2024

Sorry for the late response, @itsjeyd. I forgot about it. will be fixed shortly.

@itsjeyd
Copy link

itsjeyd commented May 31, 2024

@CodeWithEmad No problem! Looks like we've got a green build here now. Marking as ready for review 🚀

@itsjeyd itsjeyd added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label May 31, 2024
@itsjeyd
Copy link

itsjeyd commented May 31, 2024

Hey @robrap, would you or someone else from @openedx/2u-arch-bom have capacity to review this PR?

@itsjeyd itsjeyd added the needs reviewer assigned PR needs to be (re-)assigned a new reviewer label May 31, 2024
@itsjeyd
Copy link

itsjeyd commented Jun 26, 2024

Reached out to core contributors via Slack.

@robrap
Copy link
Contributor

robrap commented Jun 26, 2024

Apologies that I missed your ping. My team no longer maintains this repo. The repo should designate the maintainer if there is one. I’ll ask.

Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

The double colon is a valid way to have a piece of block text following some standrad text. What's the argument for making a switch to code blocks for all of this? Also having the prefix $ is a good indicator that the commands are meant to be run in a terminal by hand, so I'm not sure it makes sense to remove them.

@itsjeyd itsjeyd added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. needs reviewer assigned PR needs to be (re-)assigned a new reviewer labels Jun 27, 2024
@itsjeyd
Copy link

itsjeyd commented Jun 27, 2024

Thanks @robrap and @feanil.

@CodeWithEmad
Copy link
Member Author

CodeWithEmad commented Jun 28, 2024

Hi @feanil. Thanks for the review.

What's the argument for making a switch to code blocks for all of this?

Using .. code-block:: LANGUAGE provides language-specific syntax highlighting, which enhances readability, especially on GitHub. While this might not be as significant our in Sphinx docs, it ensures clarity and consistency across all platforms.

Also having the prefix $ is a good indicator that the commands are meant to be run

While the $ prefix can indicate that commands are intended to be run in a terminal, it's important to consider the overall user experience. Using a double colon in .rst files on GitHub provides some syntax highlighting, but employing a code-block will automatically add a copy button. When this button is used, having the $ at the beginning of each command forces users to manually remove that character, which can detract from their user experience. Most users will already understand that these are terminal commands, so removing the $ can streamline the process and improve usability.

image

@robrap
Copy link
Contributor

robrap commented Jun 28, 2024

Thanks @CodeWithEmad. Those seem like reasonable arguments to me, but we'll see how @feanil feels. Note that code blocks that include both a command and example output would be a different case, where having the prompt helps differentiate.

Depending on where this lands, we might want to consider adding thoughts on https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0019-bp-developer-documentation.html.

@CodeWithEmad
Copy link
Member Author

Awesome. I'll make sure Ill read it and depending on the result, I'll try to come up with changes for the OEP.

@robrap
Copy link
Contributor

robrap commented Jun 28, 2024

I don't think this is addressed much in the OEP at this point, but it could be an enhancement, depending on where we land. Also, you could use the OEP process to propose these best practices (or start a thread on the forums), if you want more than 3 opinions. :)

@itsjeyd itsjeyd added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Jul 12, 2024
@itsjeyd itsjeyd requested a review from feanil July 12, 2024 12:01
@itsjeyd itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jul 12, 2024
@feanil feanil merged commit a6a8078 into openedx:master Jul 15, 2024
8 checks passed
@openedx-webhooks
Copy link

@CodeWithEmad 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@CodeWithEmad CodeWithEmad deleted the feat/prompt branch July 15, 2024 20:30
@itsjeyd itsjeyd removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants