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

MNT Added PR template #193

Merged
merged 5 commits into from
Oct 20, 2022
Merged

MNT Added PR template #193

merged 5 commits into from
Oct 20, 2022

Conversation

merveenoyan
Copy link
Collaborator

@merveenoyan merveenoyan commented Oct 17, 2022

Partially solves #191. I used a small part of @gradio-app/gradio's template and made changes. @skops-dev/maintainers feel free to add other things on top.
(Also I think this is all you need to do if you want a template, I followed this tutorial https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository let me know if there's anything I need to do on top) We can do the same for issues as well.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

@merveenoyan
Copy link
Collaborator Author

@adrinjalali I added more stuff. Feel free to tell me to add anything else if you want it.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

all this text should also be hidden in the comment tag (as is in the link on the sklearn side).

I would start with the sklearn template and change that instead of using the gradio one. I think that one is a better written one.

Comment on lines 19 to 27
- [ ] I have performed a self-review of my own code
- [ ] I have run style formatting on my code
- [ ] I have commented my code in hard-to-understand areas
- [ ] I have made changes to docstrings and notebooks
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] I added my changes to changelog in docs/changes.rst.

Don't hesitate to ping @skops-dev/maintainers in your issues and pull requests if you don't receive a review in a timely manner. We try to review all pull requests as soon as we can.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is useful. It sounds a bit condescending to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why though, I think it's rather straightforward no?

Copy link
Member

Choose a reason for hiding this comment

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

"I performed a self review of my own code"? That's really bad. Also, not ever PR needs a change log. Also, we send PRs w/o the tests passing all the time, and that's fine, we fix them on the PR.

Comment on lines 1 to 2
Thanks a lot for contributing to skops! Please make sure you've taken a look at the contribution guidelines: https://github.com/skops-dev/skops/blob/main/CONTRIBUTING.rst You can find answers to your questions about the checklist below in this file.
Below is a template you can use to create pull requests.
Copy link
Member

Choose a reason for hiding this comment

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

line too long

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why do we have to apply line trimming on markdown doc that will appear on github inside PR opening page? I don't get it.

Copy link
Member

Choose a reason for hiding this comment

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

because it makes it easier to review and edit. It has nothing to do with how it looks on the website.

<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/skops-dev/skops/blob/main/CONTRIBUTING.rst
-->
Copy link
Member

Choose a reason for hiding this comment

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

you could add an explicit note here about linting (black, isort, pre-commit hooks, etc) maybe?

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Thanks @merveenoyan . Let's merge and see if it helps.

@adrinjalali adrinjalali changed the title Added PR template MNT Added PR template Oct 20, 2022
@adrinjalali adrinjalali merged commit c289b4a into skops-dev:main Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants