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

Updated Setup Guide and Fixed Requirements #18

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

thisisayush
Copy link

@thisisayush thisisayush commented Jun 16, 2017

  • Added Missing Requirements (Pillow) to Requirements.txt
    - Created Missing Migrations
  • Added Setup Guide for development Environment in README.md

#1

Copy link
Contributor

@CuriousLearner CuriousLearner left a comment

Choose a reason for hiding this comment

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

Hi @thisisayush !

Thanks for you work. I've mentioned my review inline.

Apart from that, please remove requirements.txt change, and all the migrations.

Migrations would be committed in the project at later point of time.

Apart from that, you're not supposed to commit your sqlite database, so please remove that.

Also, in the setup guide, add a create superuser command, with which developer can login to the admin panel.

README.md Outdated
**Note:** To deactivate the Virtual Environment, type deactivate in the shell to deactivate.
#### Clone your forked repo
```
git clone https://github.com/your_username/Confluence
Copy link
Contributor

Choose a reason for hiding this comment

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

Mention <github_username> instead of your_username

Also, add a note that people have to replace this with their github username

README.md Outdated
git clone https://github.com/your_username/Confluence
```
##### Change to develop branch
Code development should happed in 'develop' branch and not in 'master' branch
Copy link
Contributor

Choose a reason for hiding this comment

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

No, Development does not happen on develop branch. It would happen on a feature branch and PRs would be created from feature branch on their fork to the develop branch of this project.

Understand this, as, if many people are working on develop branch in their fork they have to continually rebase to get latest code and place their code on top of it (and solving merge conflicts).

We prefer solving merge conflicts when a single feature is complete (and also would be squashed to a single commit on develop branch)

README.md Outdated
```

#### Installing Project Requirements (First Time Only or on Requirement changes)
Change directory to cloned directory
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to project dir, not cloned dir

README.md Outdated
```
Use pip to install missing requirements: (if you don't have pip installed on your system, google how to install) (Windows Users should run this command Adminsitrator Shell or will get error)
```
pip install -r .\requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

no backward slashes here.

Simply goes by

pip install -r requirements.txt

README.md Outdated
pip install -r .\requirements.txt
```
#### Creating Environment Variables File
Change directory to src/confluence to create a ".env" file that will contain your enviroment variables. **Note:** This file will not ignored by git and will be available automatically on production environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

.env does not get included in git. That is the whole point of keeping settings as environment variables (because we want to prevent including confidential information on git tree)

README.md Outdated
```
Create file ".env" with following data:
```
EXPLARA_API_KEY=YOUR_EXPLARA_KEY_HERE
Copy link
Contributor

Choose a reason for hiding this comment

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

Change values to snake case with angular brackets like:

EXPLARA_API_KEY=<your_explara_key>

So that it looks different and someone reading the guide could quickly understand.

Also include a note to replace values with actual values.

README.md Outdated
```
python manage.py runserver
```
You will see something like
Copy link
Contributor

Choose a reason for hiding this comment

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

You will something like this is not needed.

Rest of the things are also not needed.

Just add that you can visit the list at localhost:8000.

README.md Outdated
Starting development server at http://127.0.0.1:8000/
Quit the server with CTRL-BREAK.
```
On successful setup you will see the second last line i.e. ' Starting Development Server at http://127.0.0.1:8000/ '. Visit this url on your web browser to see if server is started successfully. On failed startup, you will see a 'Refused to connect' error.
Copy link
Contributor

Choose a reason for hiding this comment

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

When would one get Refused to connect error?

Copy link
Author

Choose a reason for hiding this comment

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

It happens when Server is not started i.e. the runserver command get stucked at some point while starting server due to some error and the developer may think that server is started.

@thisisayush
Copy link
Author

Removing requirements.txt change will cause error while starting server because the social media app requires "Pillow" which wasn't available in requirements.txt and hence pip install -r requirements.txt will not install it and will require additional command to install in manually

Updated Setup Guide in README.md

Updated Setup Guide (required changes)
@thisisayush
Copy link
Author

Added Superuser Creation detail in README.md

README.md Outdated

### Creating a Virtual Environment
Virtual Environment allows us to have requirements installed specifically to our project instead of complete system. To create a virtual environment goto any directory where you will store your code and run the following command on terminal (linux) or Windows PowerShell (Windows)(For windows, run Windows Powershell as Administrator before running below commands)
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Please break lines after like 80-90 characters

Copy link
Contributor

Choose a reason for hiding this comment

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

goto -> go to

running below commands -> running following commands

README.md Outdated
```
Development does not happen on develop branch. It would happen on a feature branch and PRs would be created from feature branch on their fork to the develop branch of this project.

Understand this, as, if many people are working on develop branch in their fork they have to continually rebase to get latest code and place their code on top of it (and solving merge conflicts).
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rephrase this to be more readable.

Copy link
Author

Choose a reason for hiding this comment

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

Umm?

@thisisayush
Copy link
Author

Completed @aktech Requested Changes.

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.

3 participants