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.

Sign up for GitHub

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

Jump to bottom

Create PULL_REQUEST_TEMPLATE.md [skip ci] #550

Closed
wants to merge 6 commits into from
Closed

Create PULL_REQUEST_TEMPLATE.md [skip ci] #550

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
89ccae1
Create PULL_REQUEST_TEMPLATE.md
183amir May 4, 2016
33e9408
Update PULL_REQUEST_TEMPLATE.md
183amir May 4, 2016
4ae2969
Rename PULL_REQUEST_TEMPLATE.md to .github/PULL_REQUEST_TEMPLATE.md
183amir May 4, 2016
aaf53d9
Update PULL_REQUEST_TEMPLATE.md
183amir May 4, 2016
99c0472
fixes
183amir May 15, 2016
6d19f41
Add nosetests [skip ci]
183amir May 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
Recipes added with this pull request (We prefer to have one pull request per package):

- package1
- package2
Copy link
Member

Choose a reason for hiding this comment

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

Not clear on what this listing is accomplishing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It helps to see which packages are being added without having to go through the "files changed" tab.
P.S. I pushed a new commit taking your comments in consideration.


I have checked the following in my recipes:
* [ ] My files end with only 1 newline character, no more, no less.
* [ ] My recipe follow the same style as [example](https://github.com/conda-forge/staged-recipes/blob/master/recipes/example/meta.yaml) as much as possible.
Copy link
Member

Choose a reason for hiding this comment

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

We might need to clean that up to match the style we want first. It fits an older acceptable style. Also, we have been discussing having a few, which we should work on somewhere.

* [ ] The license field also specifies the number of the license if applicable (e.g. `GPLv3` instead of `GPL` or `BSD 3-clause` instead of `BSD`).
* [ ] My recipe has tests.
Copy link
Member

Choose a reason for hiding this comment

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

Should we be a bit more specific about what acceptable tests are? For instance, running make check, ctest, what have you. Also, verifying all files were installed properly. Possibly, a simple program to exercise the package (especially if there is no make check, etc.).

* [ ] I have looked at [pinned packages](https://github.com/conda-forge/staged-recipes/wiki/Pinned-dependencies) and pinned those packages as stated.
* [ ] The `summary` just explains the package and does not include the package name. (e.g. Instead of saying `Jupyterhub is a multi-user server for Jupyter notebooks` just say `Multi-user server for Jupyter notebooks`)

If you build for Windows too:
* [ ] I have read [VC features](https://github.com/conda-forge/staged-recipes/wiki/VC-features) and implemented.

If recipe uses make or cmake or ctest:
* [ ] I have also added `make check` or similar if applicable.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry hadn't gotten here yet. This looks good.


If recipe builds a library:
* [ ] I have enabled both static and shared libraries.

If recipe builds some C/C++:
* [ ] I have not included `gcc` or `libgcc` in `requirements`. Exceptions can be made but must be tested first with `gcc`/`clang` that is already installed in our CI machines.
Copy link
Member

@jakirkham jakirkham May 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Would rephrase. Currently Fortran and OpenMP are ok uses of the packages. So, we should make an exception. Though this is something we strive to eliminate.


If it is a Python PyPI package:
* [ ] I have used `python` to install it and my recipe has these elements:
```yaml
build:
script: python setup.py install --single-version-externally-managed --record=record.txt
requirements:
build:
- python
- setuptools
run:
- python
```
* [ ] Test commands such as `nosetests package` or `py.test` are added:
```yaml
test:
requires:
- nose

commands:
- nosetests -sv package
Copy link
Member

@jakirkham jakirkham May 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

IMHO the point of testing is not really to run CI for the packages that are given to us. Instead it is here to guarantee the following things.

  1. The compiler didn't mess something up.
  2. Something didn't go wrong in the linking stage.
  3. That the package will work in the environment defined.

The first two don't really apply to pure Python packages so let's ignore that for now. As far as solving the last one, importing modules seems to be sufficient for the most part. Running the full test suite is something the developer(s) hopefully are doing on releases at least if not during development as part of CI. Thus, there isn't really much value in us doing it to for pure Python packages. If the Python code does have compiled portions, that is a totally different story.

In the end, there may be a few packages that need something like run_test.py to do a few more things, but that will probably be determined after it is a feedstock.

In short, I don't think this should be a hard requirement for pure Python packages.

```