edx-cookiecutters
edx-cookiecutters copied to clipboard
feat: prompt messages
Merge checklist:
Check off if complete or not applicable:
- [x] Changelog record added
- [x] Documentation updated (not only docstrings)
- [x] Fixup commits are squashed away
- [x] Unit tests added/updated
- [x] Manual testing instructions provided
- [x] 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.
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.
Hey @CodeWithEmad, thank you for this contribution! I'll line them up for a test run now.
Hey @CodeWithEmad, looks like you got some failing tests here. Let us know when you've had a chance to address those.
Sure @itsjeyd. My changes were pretty harmless :) I'll look into it.
@itsjeyd Tests are failing due to missing packages, possibly caused by Dependabot (also see https://github.com/openedx/edx-cookiecutters/pull/448). Fixed in https://github.com/openedx/edx-cookiecutters/pull/451. Probably mine will be fixed too after merging 451.
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 :)
Hey @CodeWithEmad, just checking in to see if you're still planning to continue working on this PR?
Sorry for the late response, @itsjeyd. I forgot about it. will be fixed shortly.
@CodeWithEmad No problem! Looks like we've got a green build here now. Marking as ready for review 🚀
Hey @robrap, would you or someone else from @openedx/2u-arch-bom have capacity to review this PR?
Reached out to core contributors via Slack.
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.
Thanks @robrap and @feanil.
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.
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.
Awesome. I'll make sure Ill read it and depending on the result, I'll try to come up with changes for the OEP.
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. :)
@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.