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

feat(hypothesis): Added hypothesis as an option #134

Merged
merged 45 commits into from
Jun 29, 2023

Conversation

ayeankit
Copy link
Contributor

Pull Request description

Issue #23

How to test these changes

  • ...

Pull Request checklists

This PR is a:

  • bug-fix
  • new feature
  • maintenance

About this PR:

  • it includes tests.
  • the tests are executed on CI.
  • the tests generate log file(s) (path).
  • pre-commit hooks were executed locally.
  • this PR requires a project documentation update.

Author's checklist:

  • I have reviewed the changes and it contains no misspelling.
  • The code is well commented, especially in the parts that contain more complexity.
  • New and old tests passed locally.

Additional information

Reviewer's checklist

Copy and paste this template for your review's note:

## Reviewer's Checklist

- [ ] I managed to reproduce the problem locally from the `main` branch
- [ ] I managed to test the new changes locally
- [ ] I confirm that the issues mentioned were fixed/resolved .

@@ -1,4 +1,5 @@
"""Tests for `{{ cookiecutter.project_slug }}` package."""
{%- if cookiecutter.use_pytest == "yes" -%}
Copy link
Member

Choose a reason for hiding this comment

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

could you revisit this file again?

keep in mind that it would be possible to have both pytest and hypothesis.
also please, check the result locally to see what is the final result for this file for all the 3 possibilities (pytest, hypothesis, pytest + hypothesis).

also pay attention on the jinja2 modifications %-/-%, %+/+% or just % .. and the combinations among them

@xmnlab xmnlab changed the title Feat(hypotheis): Added hypothesis as an option feat(hypothesis): Added hypothesis as an option Jun 20, 2023
@ayeankit
Copy link
Contributor Author

@xmnlab and @Saransh-cpp I do not know why its failing the main/test (pull_request). Can you review it?

@xmnlab
Copy link
Member

xmnlab commented Jun 20, 2023

@xmnlab and @Saransh-cpp I do not know why its failing the main/test (pull_request). Can you review it?

all the files need to end with one empty line, no more, no less.
and this PR has 2 files that that problem: https://github.com/osl-incubator/scicookie/actions/runs/5328022194/jobs/9652160700?pr=134#step:6:21

Fixing src/scicookie/cookiecutter.json
Fixing src/scicookie/{{cookiecutter.project_slug}}/pyproject.toml

these issues should be caught by the pre-commit locally. check if you have it installed with pre-commit install

@xmnlab
Copy link
Member

xmnlab commented Jun 20, 2023

and run locally pre-commit run --all-files --verbose

.PHONY:test
test: ## run tests quickly with the default Python
pytest
{% endif %}
{%- endif +%}
{% if cookiecutter.use_hypothesis == "yes" -%}
Copy link
Member

Choose a reason for hiding this comment

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

one problem here is that,
if pytest == yes and hypothesis == yes, it will create the test target twice.

@ayeankit
Copy link
Contributor Author

hey @xmnlab and @Saransh-cpp again these test cases are failing , I am using pre-commit locally but here it not passing on gh workflows. I think I am not correctly dealing with makefile when pytest + hypothesis both options are selected. Can you look into it?

Copy link
Collaborator

@Anavelyz Anavelyz left a comment

Choose a reason for hiding this comment

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

@ayeankit, it seems that the docstrings are giving problems, I left you some suggestions. I hope it works.

@ayeankit
Copy link
Contributor Author

@Anavelyz after changing the auto-string, it's still failing the gh actions. Can you see why its failing?

@xmnlab
Copy link
Member

xmnlab commented Jun 26, 2023

I didn't check the CI error ..but if it is just about the docstring .. just keep it shorter.
as it is just a placeholder, it does't matter too much.

@ayeankit
Copy link
Contributor Author

@xmnlab I added the documentation in guide.md , contributing.md and Readme.md files as suggested. Can you review it once again?

README.md Outdated
Comment on lines 29 to 31
- Flexible build system selection: Choose between popular build systems like
[Poetry](https://python-poetry.org/) or [Flit](https://flit.pypa.io) based
on your preference.
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 shouldn't have been removed.

docs/guide.md Outdated
Comment on lines 38 to 40
- [**Pytest**](https://docs.pytest.org/en/7.3.x/contents.html): is a powerful property-based testing library for Python. It generates test cases based on properties, uncovering bugs and corner cases. Learn more in the Hypothesis documentation.

- [**Hypothesis**](https://hypothesis.readthedocs.io/): is a powerful property-based testing library for Python. It generates test cases based on properties, uncovering bugs and corner cases. Learn more in the Hypothesis documentation.
Copy link
Member

Choose a reason for hiding this comment

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

Some formatting issues and repetition here 🙂

README.md Outdated Show resolved Hide resolved
ayeankit and others added 3 commits June 28, 2023 20:37
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
@ayeankit
Copy link
Contributor Author

@xmnlab I changed the files as suggested by @Saransh-cpp. Can you review it once again?

docs/guide.md Show resolved Hide resolved
docs/guide.md Show resolved Hide resolved
Copy link
Member

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

@ayeankit in general it looks good. thanks!
could you just improve a bit the documentation in the template side please?

you can get some ideas from here https://github.com/osl-incubator/scicookie/pull/136/files#diff-f1c71bca02a04da875655ec4b14b5bc1ed65d7c025276bd97efbf97bc464a3b1

In general, you should try to see the documentation from the perspective of a new developer. Maybe a new developer would not know what is pytest or hypothesis.

so maybe 1 or 2 paragraphs about that and a link to the official web/documentation page would be great.

other than that, it looks great :)

ayeankit and others added 4 commits June 29, 2023 21:27
Co-authored-by: Ivan Ogasawara <ivan.ogasawara@gmail.com>
Co-authored-by: Ivan Ogasawara <ivan.ogasawara@gmail.com>
@ayeankit
Copy link
Contributor Author

@xmnlab I changed the files as suggested . Can you review it once again?

docs/guide.md Outdated Show resolved Hide resolved
docs/guide.md Outdated Show resolved Hide resolved
README.md Outdated
- Flexible build system selection: Choose between popular build systems like
[Poetry](https://python-poetry.org/) or [Flit](https://flit.pypa.io) based
on your preference.
- Release workflow with semantic release and github actions
Copy link
Member

Choose a reason for hiding this comment

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

This looks unrelated to this PR 🙂

Should be okay though, just moved down

ayeankit and others added 3 commits June 30, 2023 01:20
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Copy link
Member

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

thanks for working on that @ayeankit !

@xmnlab xmnlab merged commit 791ca16 into osl-incubator:main Jun 29, 2023
@xmnlab
Copy link
Member

xmnlab commented Jun 29, 2023

@ayeankit next time, please provide more information in the Pull Request description section in the first comment in the PR. thanks

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

🎉 This PR is included in version 0.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants