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

docs: update readme #295

Merged
merged 7 commits into from
Nov 29, 2023
Merged

Conversation

FredrikOseberg
Copy link
Collaborator

This PR includes documentation that instructs the user on how to setup their development environment in order to effeciently run the tests.

@coveralls
Copy link

coveralls commented Nov 29, 2023

Coverage Status

coverage: 96.905%. remained the same
when pulling 3b5c947 on docs/update-readme-with-dev-instructions
into a0dfabd on main.

Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

Nice! Useful improvement.

I think we should add the mention of a venv here, everything else is optional details

README.md Outdated
Then run:

```
pip3 install -r requirements.txt
Copy link
Member

Choose a reason for hiding this comment

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

This can have some very nasty side effects if your OS has a Python interpreter baked in. Usually you'd put down a venv first (this ought to be common knowledge for most Python devs but if we want to be explicit):

Linux & Max:

python3 -m venv venv
source venv/bin/activate 

Windows + cmd:

python -m venv venv
venv\Scripts\activate.bat

Powershell:

python -m venv venv
venv\Scripts\activate.bat

Then for any of them:
pip install -r requirements.txt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Brilliant, thanks!

Updating the PR with your suggestions. For my curiosity, what side effects can happen if you don't put down a viritual environment?

Sidenote: I got an error when attempting to run tests after running pip install -r requirements.txt. I also had to run pip install UnleashClient which I thought was weird. Does that track?

Copy link
Member

Choose a reason for hiding this comment

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

Great question! Mac and Linux come with a Python version (actually 2 of the suckers) by default, which the OS uses as a scripting layer. The most likely thing that happens if you have a few Python projects is that your packages conflict and another project breaks. The worst possible case (which is unlikely), is that you override a dependency that your OS requires to function correctly

Setting up a venv makes Python and pip work more like npm does by default - every project gets its own set of dependencies and they're isolated from each other

Hmm, the pip install UnleashClient shouldn't have been necessary, that should be adding a dependency of itself. I want to say setting up a venv should make that go away

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Verified and removed. Thanks!

README.md Outdated

```
cd tests/specification_tests
git clone https://github.com/Unleash/client-specification
Copy link
Member

Choose a reason for hiding this comment

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

There's a shell script in scripts/get-spec.sh, which tox uses to download the appropriate version of the client spec and put it in the right place. You can run that from the root. Might be worth mentioning

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
FredrikOseberg and others added 5 commits November 29, 2023 14:07
Co-authored-by: Simon Hornby <liquidwicked64@gmail.com>
Co-authored-by: Simon Hornby <liquidwicked64@gmail.com>
@FredrikOseberg FredrikOseberg merged commit f4679ab into main Nov 29, 2023
37 checks passed
@FredrikOseberg FredrikOseberg deleted the docs/update-readme-with-dev-instructions branch November 29, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants