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
Update docs/development #1416
Update docs/development #1416
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcfidalgo - Thank you!
I've left some inline comments with some suggestions for small changes.
cd gammapy | ||
conda env create -f environment-dev.yml | ||
source activate gammapy-dev | ||
# for conda versions >=4.4.0 you may have to execute | ||
#'conda activate gammapy-dev' instead | ||
git remote add gammapy git@github.com:gammapy/gammapy.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to use the names origin
and upstream
for the remotes.
That's what most projects on Github use, and also ctapipe.
I see that Astropy has changed to other names, which is unfortunate IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerning names of remotes again: I see that the Astropy contributor docs used astropy
for upstream
and YOUR_USERNAME
since over four years ago, and that there is a reasoning:
astropy/astropy#1712 (comment)
These names are clearer and what's used in https://hub.github.com/ which many people use when working with Github.
On the other hand, e.g. scikit-learn (http://scikit-learn.org/stable/developers/contributing.html) also uses upstream
and origin
, just like ctapipe
.
@kosack - I guess you want to stick with upstream
and origin
?
So this is difficult what to recommend, and something people are always confused about.
In any case, I'd suggest showing $ git remote -v
to show what the two remotes are when following our guidelines, and also to give people the command they need to find out what's what.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcfidalgo - I'd suggest that you go ahead and finalise this PR, i.e. just keep the remote names as used by Astropy for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a couple of sentences about the repo names, i think it should be sufficient to avoid confusion.
docs/development/intro.rst
Outdated
# for conda versions >=4.4.0 you may have to execute | ||
#'conda activate gammapy-dev' instead | ||
git remote add gammapy git@github.com:gammapy/gammapy.git | ||
git branch my-gammapy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one line about branches and a reference to the Make a working example section, and used a [branch-name]
placeholder.
docs/development/intro.rst
Outdated
|
||
.. code-block:: bash | ||
|
||
python setup.py build_ext --inplace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think people will be confused and execute both the python setup.py build_ext --inplace
and the pip install -e .
, but actually pip install -e .
executes python setup.py build_ext --inplace
and as a second step puts a link from the site-packages to the local folder. So I'd suggest to just remove the python setup.py build_ext --inplace
, it should not be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not aware that pip install -e .
executes python setup.py build_ext --inplace
. Thanks for pointing that out! I just removed the first command.
- Implemented Christoph's suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcfidalgo - Looks good. Thank you!
Just a small update of the docs/development.
I basically just wanted to practice the development workflow of gammapy.