8000 GitHub Templates for issue and pull request by mathdugre · Pull Request #479 · boutiques/boutiques · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

GitHub Templates for issue and pull request #479

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
Merged
merged 6 commits into from
May 23, 2019
Merged

GitHub Templates for issue and pull request #479

merged 6 commits into from
May 23, 2019

Conversation

mathdugre
Copy link
Contributor
@mathdugre mathdugre commented May 8, 2019 •
edited
Loading

Created a set of template to make the issues and pull request more uniform.

Purpose

This should help in making issues (pull request) more comprehensible thus improving the reviewing process. Also this aims at improving the code base quality by establishing standard and make sure that their adoption is made.

Minor suggestions

  • Document the API according to PEP 257

Major suggestions

  • Making all code compliant with the PEP 8 code style
  • Adopting sphinx as a documentation lint
  • Using type annotation with mypy as a linter

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.714% when pulling 656014b on mathdugre:template into aecc71c on boutiques:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.714% when pulling 656014b on mathdugre:template into aecc71c on boutiques:develop.

@coveralls
Copy link
coveralls commented May 8, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 93.714% when pulling 329767a on mathdugre:template into aecc71c on boutiques:develop.

@glatard
Copy link
Member
glatard commented May 8, 2019

Hi @mathdugre, thanks for doing this!

The templates look great to me! Just out of curiosity, did you take them from somewhere in particular?

I definitely agree with documenting the API and putting it on sphinx. This should be part of #254.

As for code style, the code should already be compliant with PEP8, and we test that using pycodestyle in Travis. Do you think we still need to make it a check box in the PR template?

As for mypy, I don't know. I'm not sure that we want to do this at all in Boutiques, it's quite a major change... Maybe we can have this conversation in a separate issue so that it doesn't hinder the merging of this one.

@gkiar @ramou

@mathdugre
Copy link
Contributor Author

Hi @glatard for the issue templates I was strongly inspired by this one. For the PR templates I got inspiration from a few ones and cherry picked the features I liked from them.

I will start working on the documentation.

For pep8, I pointed it out since I came across a few function CapWord convention and unsorted import in some modules. Also some tool like black could ease the process of formating the code to comply with PEP8. I'll open another issue for discussion.

As for mypy I totally agree that it would be a major change. I'll open an issue to discuss the consequences that it would imply.

For now I'll remove PEP8 and mypy from the checklist.
Also, do you think a discussion template could be useful?

@glatard
Copy link
Member
glatard commented May 10, 2019

As for PEP8, pycodestyle could be made more stringent in Travis. Black also looks relevant, however, we should first update all the cod 8000 e before requesting that new PRs do that.

The same goes with API documentation and Sphinx: before we have them in the template, we should make sure that the current code complies to them, otherwise it will be difficult for developers to adopt a standard which is currently not enforced.

For bug reports, I'm worried that the template is a bit verbose and will discourage people reporting bugs. Maybe we can indicate that some fields are optional, in their title?

@erinb90
Copy link
Contributor
erinb90 commented May 21, 2019

Hey @mathdugre, this is really nice! If possible, I would also add templates for refactoring existing code and for creating/updating documentation.

@glatard
Copy link
Member
glatard commented May 23, 2019

Let's merge this as it's already super useful! I agree with @erinb90, we could have templates to refactor code and documentation. Also, the bug PR template should include tests too.

@glatard glatard merged commit 77c8cae into boutiques:develop May 23, 2019
@mathdugre
Copy link
Contributor Author

I will work on the refactoring and documentation template.

Also, the bug PR template should include tests too.

@glatard I am not sure to understand what is meant here. Is it the check box with label DO Created unit test for the feature. ?

@glatard
Copy link
Member
glatard commented May 23, 2019

yes, this it what I meant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mathdugre @coveralls @glatard @erinb90
0