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 Venv official docs link #11040

Merged
merged 1 commit into from
Oct 13, 2023
Merged

Conversation

krishvsoni
Copy link
Contributor

Fixes #...

In the README file before installation and setup of the project, there is a line that says to run the module in the virtual enviorment, so in that virtual enviorment i had given official documentation of python virtual enviorment.

Please check the following:

  • Do the tests still pass?1
  • Does the code comply with the style guide?
    • Run make lint from the Wagtail root.
  • For Python changes: Have you added tests to cover the new/fixed behaviour?
  • For front-end changes: Did you test on all of Wagtail’s supported environments?2
    • Please list the exact browser and operating system versions you tested:
    • Please list which assistive technologies 3 you tested:
  • [✔️] For new features: Has the documentation been updated accordingly?

Please describe additional details for testing this change.

image

Footnotes

  1. Development Testing

  2. Browser and device support

  3. Accessibility Target

@squash-labs
Copy link

squash-labs bot commented Oct 12, 2023

Manage this branch in Squash

Test this branch here: https://krishvsonikrishvsoni-patch-2-z0bas.squash.io

Copy link
Member

@lb- lb- left a comment

Choose a reason for hiding this comment

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

Thanks @krishvsoni - a few bits of feedback and suggestions for changes.

I'm not 100% sure we want this as we do try to keep the Readme brief and generic. However, maybe a slight rewording could help with the link.

I'll let another core team also review to confirm we are happy with this.

Thanks for the PR though.

@@ -48,11 +48,12 @@ Find out more at [wagtail.org](https://wagtail.org/).

Wagtail works with [Python 3](https://www.python.org/downloads/), on any platform.

To get started with using Wagtail, run the following in a virtual environment:
To get started with using Wagtail, run the following in a [virtual environment](https://docs.python.org/3/tutorial/venv.html):
Copy link
Member

Choose a reason for hiding this comment

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

This may not be exactly right, there are a few different ways to set up a virtual environment.

We may cause more confusion by being specific here.

I'll let other core team chime in though. We do intentionally keep this brief as there main docs are the place to get full instructions. https://docs.wagtail.org/en/stable/getting_started/tutorial.html#create-and-activate-a-virtual-environment

Maybe we can write

Suggested change
To get started with using Wagtail, run the following in a [virtual environment](https://docs.python.org/3/tutorial/venv.html):
To get started with using Wagtail, run the following in a virtual environment such as [venv](https://docs.python.org/3/tutorial/venv.html):

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer @krishvsoni's original version here, and I'd point to the Diataxis framework's advice on tutorials to back this up - Ignore options and alternatives.

Anyone who's knowledgeable / opinionated enough to have a preferred approach to creating virtual environments will happily disregard the link and do their own thing; and for everyone else, knowing that alternatives to venv exist (but not anything about them) doesn't add any value. And, as you say, people who are not familiar with virtual environments at all are better served by the full tutorial.

Copy link
Member

Choose a reason for hiding this comment

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

Good call, thank you.

@krishvsoni if you are OK to clean up the other parts of the PR (just revert accidental/unrelated changes), we should be good to go on this one.

README.md Outdated

![Installing Wagtail](.github/install-animation.gif)

```sh

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this accidental whitespace change.

@@ -1,12 +1,9 @@
from collections import OrderedDict

Copy link
Member

Choose a reason for hiding this comment

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

Please remove the Changes to this file, I don't think this relates to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this changes in another branch so here in this branch it's about venv only there is no other change but still I verify and change it.

@lb-
Copy link
Member

lb- commented Oct 12, 2023

Please also update the title to be clearer about what you're doing and also remove the 'fixes...' text from the description.

We have a section about creating a PR in our contribution guide. https://docs.wagtail.org/en/stable/contributing/first_contribution_guide.html#submitting-a-pull-request

Copy link
Member

@lb- lb- left a comment

Choose a reason for hiding this comment

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

Thanks @krishvsoni I have updated your branch with the changes, I know it's a bit tricky getting used to git branches but I could see you had created this patch-2 branch from your other patch-1 branch.

All good though, fixed up and I will merge this in.

Thanks for helping us improve Wagtail, I have added you to the contributors list as 'Krish Soni' but let me know if you want a different name listed.

@lb- lb- merged commit 7161729 into wagtail:main Oct 13, 2023
13 of 16 checks passed
@krishvsoni krishvsoni changed the title Krishvsoni patch 2 Added Venv official docs link Oct 13, 2023
@krishvsoni
Copy link
Contributor Author

Thanks @krishvsoni I have updated your branch with the changes, I know it's a bit tricky getting used to git branches but I could see you had created this patch-2 branch from your other patch-1 branch.

All good though, fixed up and I will merge this in.

Thanks for helping us improve Wagtail, I have added you to the contributors list as 'Krish Soni' but let me know if you want a different name listed.

Happy to contribute in the Wagtail, I am so grateful to you guys @lb- and @gasman for helping me 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants