Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

Included contributing section into README: #13

Merged
merged 3 commits into from
May 3, 2022

Conversation

fsenf
Copy link
Member

@fsenf fsenf commented Apr 14, 2022

Based on the discussions in PR #10 & issue #8, I added a small description to our README containing:
(1) requirements for PRs
(2) example data upload

(1) requirements for PRs
(2) example data upload
@fsenf fsenf requested a review from w-k-jones April 14, 2022 11:37
@JuliaKukulies
Copy link
Member

Great! Maybe the PR requirements could also be in form of a pull_request_template.md, to be consistent with the other repo and that these checks will automatically appear when submitting a PR?

@fsenf
Copy link
Member Author

fsenf commented Apr 19, 2022

@JuliaKukulies: Nice idea! I added a PR template based on your link!

@fsenf fsenf assigned JuliaKukulies and unassigned w-k-jones Apr 19, 2022
@fsenf fsenf requested review from JuliaKukulies and removed request for w-k-jones April 19, 2022 12:45
@fsenf
Copy link
Member Author

fsenf commented Apr 19, 2022

@JuliaKukulies : Would you like to proceed the review?

Copy link
Member

@JuliaKukulies JuliaKukulies left a comment

Choose a reason for hiding this comment

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

Hey @fsenf, two small comments from me (see below). Otherwise it looks good, also with the PR template. Thanks for adding this one! I hope it is not an overkill of instructions now, but I think it is useful to have this PR check list to facilitate the contribution and review.

README.md Outdated
* **with your suggestions to change the documentation elements**, e.g. `index.rst`
* please care that sphinx-based rendering is still working correctly

* **or with combination of both**
Copy link
Member

Choose a reason for hiding this comment

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

I think this line would slightly confuse me, should there be separate pull requests or is it OK to have the addition of the notebook and changes to index.rst in one?

README.md Outdated
* please check if your notebook is running in the recommended standard conda enviroment [see here]((docs/RunningTutorials.md) and update the [list of required python packages](./requirements.txt) if needed

* **with your suggestions to change the documentation elements**, e.g. `index.rst`
* please care that sphinx-based rendering is still working correctly
Copy link
Member

Choose a reason for hiding this comment

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

Maybe provide a link on how to check this or in general how jupyter notebook works with Sphinx (e.g. https://docs.readthedocs.io/en/stable/guides/jupyter.html) ?

@fsenf
Copy link
Member Author

fsenf commented Apr 21, 2022

@JuliaKukulies : Thanks for your suggestions! I removed the bullet that confused you & I added a small docu on how to run sphinx-based rendering locally. Please have a look ...

@fsenf
Copy link
Member Author

fsenf commented May 3, 2022

@JuliaKukulies : PR ready to merge? What do you think?

@JuliaKukulies
Copy link
Member

Sorry for delaying this @fsenf, but I am happy with it now and would say go ahead and merge!

@fsenf
Copy link
Member Author

fsenf commented May 3, 2022

@JuliaKukulies : Great, and be convinced that I am always slower than you ;-)

@fsenf fsenf merged commit b04f4be into tobac-project:master May 3, 2022
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.

3 participants