Skip to content
This repository was archived by the owner on Jan 19, 2018. It is now read-only.

Conversation

@surajssd
Copy link
Collaborator

@surajssd surajssd commented Feb 8, 2016

Closes issue#542

Moved the Developers section from README.md to CONTRIBUTING.md
This makes easy for a developer to find all the information in one place.

CONTRIBUTING.md Outdated
$ python setup.py develop
$ pip install -r test-requirements.txt
$ alias atomicapp=<path-to-venv>/venv/bin/atomicapp
$ alias sudo='sudo '
Copy link
Member

Choose a reason for hiding this comment

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

imo, this is too complicated and not friendly for development, we added make install, can't we just use that?

i'd be open to modify Makefile to add a "virtual" environment, ex. make dev as an alternative that we re-iterate what you just put here. saves having modify each alias every time. I'm not too familar to people coding via virtualenv, but giving people the option to do so would be cool

but I'm against this way. each time a person wants to develop they'd have to setup all these aliases again each time plus it's difficult >.>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes will look into Makefile and try to make changes accordingly.

@surajssd surajssd force-pushed the developers-to-contributing branch from ee67c2e to e74b67d Compare February 9, 2016 05:38
Moved the `Developers` section from README.md to CONTRIBUTING.md
This makes easy for a developer to find all the information in one place.
@surajssd surajssd force-pushed the developers-to-contributing branch from e74b67d to 917b195 Compare February 9, 2016 05:45
python setup.py develop; \
echo "alias atomicapp=~/.virtualenvs/atomicapp/bin/atomicapp" >> ~/.virtualenvs/atomicapp/bin/postactivate; \
echo "alias sudo='sudo '" >> ~/.virtualenvs/atomicapp/bin/postactivate; \
echo "unalias atomicapp && unalias sudo" >> ~/.virtualenvs/atomicapp/bin/postdeactivate
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool 👍

@rtnpro
Copy link
Contributor

rtnpro commented Feb 9, 2016

@surajssd improve the commit message, and we are good to merge.

The first line of your commit message should be a summary of the fix (preferably, less than 60 chars). Followed by an optional summary, and reference to Github issue.

@cdrage
Copy link
Member

cdrage commented Feb 9, 2016

Let's wait for discussion from @dustymabe @goern or @vpavlin

I don't think this is the best way to introduce someone to development.

At the moment the current workflow is:

git clone projectatomic/atomicapp . && atomicapp
# code away
sudo make install

Instead of running this in a virtualenv.

Please don't merge for now.


## Developers

First of all, fork the [repository](https://github.com/projectatomic/atomicapp) and clone your github repository:
Copy link
Member

Choose a reason for hiding this comment

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

Fork [atomicapp](https://github.com/projectatomic/atomicapp) and clone your GitHub repo:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Acknowledged

@surajssd
Copy link
Collaborator Author

surajssd commented Feb 9, 2016

Okay I will wait for others to comment and then we can move ahead.

@dustymabe
Copy link
Contributor

A few things here. It is much easier for reviews and for reading through change history if you don't move code (or docs in this case) and also change the content in the same commit. It's usually better to copy the code in one commit and then make changes to it in the next.

As for the virtualenv stuff. I don't mind having a section that explains how to use it with atomicapp but I'd rather not make it a requirement. I'm not sure having it be part of the Makefile is desirable. The virtualenv stuff should also be in a separate commit.

To summarize:

  • Let's break this up into separate commits
  • Let's use descriptive commit messages
  • After you update the PR we'll review again and see where the virtualenv stuff fits in.

Also charlie should be getting some changes in for his PR on the readme and they should go in soon so you may want to wait to update this PR til after that.

@surajssd
Copy link
Collaborator Author

First off I will wait for the @cdrage 's PR on README once that gets pulled further I will make respective changes.

About dividing the changes into 'move' and 'change', should this be accompained with new issue as well?

And one suggestion in make file we can make something like 'install for development' with virtualenv and without virtualenv, which ever folks choose viz make develop and make develop virtualenv or something with better names!

@dustymabe
Copy link
Contributor

First off I will wait for the @cdrage 's PR on README once that gets pulled further I will make respective changes.

Thanks!

About dividing the changes into 'move' and 'change', should this be accompained with new issue as well?

No new issue needed.

And one suggestion in make file we can make something like 'install for development' with virtualenv and without virtualenv, which ever folks choose viz make develop and make develop virtualenv or something with better names!

perhaps we could go for something like that. I'll review your proposal when you update this PR and we'll see where to go with it.

@surajssd surajssd mentioned this pull request Feb 23, 2016
@cdrage
Copy link
Member

cdrage commented Mar 7, 2016

Hey @surajssd I've fixed this issue with #576 :) Let me know if you'd like anything added to the CONTRIBUTING.md section!

@cdrage cdrage closed this Mar 7, 2016
@surajssd
Copy link
Collaborator Author

surajssd commented Mar 8, 2016

@cdrage 👍 great work on the docs side.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants